-
Notifications
You must be signed in to change notification settings - Fork 52
Add static memory cache for discovered resources #1132
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements static memory caching for command discovery to avoid redundant discovery operations when using adapted resources. The engine previously performed discovery multiple times for nested configurations, which was inefficient.
Key changes:
- Adds a static cache module using global statics with mutexes for thread-safe access
- Refactors
CommandDiscovery
to use the cache instead of instance fields - Replaces direct field access with cache getter/setter functions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
dsc_lib/src/discovery/mod.rs | Adds the new cache module to the discovery module |
dsc_lib/src/discovery/command_discovery_cache.rs | Implements the static cache with thread-safe global storage for discovered resources |
dsc_lib/src/discovery/command_discovery.rs | Refactors CommandDiscovery to use the static cache instead of instance fields |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
42b31c8
to
6e62bb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions, and some Rust I need to go look up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I forgot to approve this.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
04e3129
to
d929b6e
Compare
@andyleejordan FYI - the mutex with macros was resulting in a deadlock, changing to using |
I'm curious where exactly the deadlock was. If it didn't deadlock in the same way with the wrappers, that indicates we're acquiring the lock in the same scope in some place where we meant not to hold the lock (and so were implicitly depending on the lock expiring inside the wrapper)...ah, probably where we're reading to check
If that's the case, then I agree, |
PR Summary
When using adapted resources, the engine will dynamically generate a nested configuration. Each configuration (outer and inner) would incur the cost of performing discovery. This change makes command discovery retain an in-memory static cache so that any multiple discoveries will just use the previous one.