Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache Descriptor Set Path #202

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Cache Descriptor Set Path #202

merged 7 commits into from
Jul 30, 2024

Conversation

ifadams
Copy link
Contributor

@ifadams ifadams commented Jul 25, 2024

Hey Folks,

This is a draft PR for the descriptor set path caching.

Currently, to the best of my knowledge, we can't actually delete a descriptor set after creation so this is a fairly minimal change.

It relies on the fact that a descriptor set path, as well as its dimensionality, will never change after creation. This lets us bypass the database query after the initial caching of the path and dimensions for a named set.

@ifadams ifadams changed the base branch from master to develop July 25, 2024 19:16
cwlacewe
cwlacewe previously approved these changes Jul 25, 2024
Copy link
Contributor

@cwlacewe cwlacewe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Minor removal of extra lines requested.

src/RSCommand.h Outdated Show resolved Hide resolved
Copy link
Contributor

@s-gobriel s-gobriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it is necessary to also clear the cache when the index is deleted in

DescriptorSet::~DescriptorSet() { delete _set; }

@ifadams
Copy link
Contributor Author

ifadams commented Jul 26, 2024

Do you think it is necessary to also clear the cache when the index is deleted in

DescriptorSet::~DescriptorSet() { delete _set; }

This is a concern of mine longer term, but it doesn't look like the "delete" is actually externally connected at the moment. i.e. I dont see any VDMS level logic that lets us fully remove an index. At most, with some gymnastics we might be able to remove the descriptor set reference from the graph database, but even thats a little murky.

@s-gobriel do you know if there's an example or call we can check to see if a client can actually trigger a delete of either the index or the referencing PMGD node?

formatting cleanup

Co-authored-by: Chaunte W. Lacewell <[email protected]>
s-gobriel
s-gobriel previously approved these changes Jul 26, 2024
Copy link
Contributor

@s-gobriel s-gobriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct @ifadams. I checked the whole source and the memory resident index is never deleted, so I don't see a problem with the cached path.

cwlacewe
cwlacewe previously approved these changes Jul 26, 2024
@ifadams ifadams marked this pull request as ready for review July 26, 2024 17:23
Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

@sys-vdms sys-vdms dismissed stale reviews from cwlacewe and s-gobriel via 85f9db2 July 26, 2024 17:51
Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

src/RSCommand.h Outdated
@@ -49,6 +50,10 @@ class RSCommand {
const std::string _cmd_name;
std::map<std::string, int> _valid_params_map;

static tbb::concurrent_unordered_map<std::string, std::string>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to place this DescriptorSet specific cache in the base RSCommand class? Won't it be better placed in DescriptorsCommand class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the time there was a specific reason (actually did the dev work weeks ago) and was having some compilation issues with it in the subclass.

I can't for the life of me remember the specifics though.... if its causing some heartburn I can try moving it back to the subclass. There's some vaguely potential value of having it in the super class for more general access for some hand-wavey other future dev but from a functional standpoint right now, meh

Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

Copy link

Target CPP Coverage: 64.4418%
Source CPP Coverage: 64.3906%

Target Python Coverage: 98%
Source Python Coverage: 98%

@ifadams ifadams merged commit 0555e37 into develop Jul 30, 2024
9 checks passed
@cwlacewe cwlacewe added this to the v2.10.0 Tasks milestone Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Descriptor Set Optimizations: Cache Descriptor Set File Path from Metadata
5 participants