Improve Command Caching by Aliasing Commands #2304
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This simplifies the logic significantly. Some downsides to this: - its not bidirectional, so 'C:\dotnet' gets aliased to 'dotnet' but any cached stuff for 'C:\dotnet' is not saved and reused by 'dotnet'. But this is a problem with the previous approach. - Doing it this way allows us to future proof commands. e.g. list sdks, list runtimes, etc. having it bi-directional requires us to see what commands were already cached with the previous command key, which would mean we'd need to store all of the cached commands or search the entire cache map since the options etc are part of what gets cached, we can't just search via the command root, unless we separate the data structure into a 2 part lookup, but that will add further issues. consider linux edge cases here, this should be ok for now.
log seems to show its not working. Need to check this and added test. maybe the in place edit of the object doesnt work
Member
Author
|
On my machine: List Runtimes took: 1495.66 ms Total time saved: 1493 ms ( in theory bc the thread is shared, so some x time less than this, but this was time blocking the entire component) Btw, --info took 5713.15 🤪 |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves the caching mechanism for dotnet commands by aliasing equivalent commands and ensuring deterministic options serialization. Key changes include:
- Updating test cases to use a defined cache TTL and verifying alias logic.
- Enhancing the LocalMemoryCacheSingleton to incorporate command root aliasing and sorted option keys in cache key generation.
- Adjusting DotnetPathFinder and DotnetConditionValidator to support aliasing and improved runtime command execution.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vscode-dotnet-runtime-library/src/test/unit/LocalMemoryCacheSingleton.test.ts | Updated test values to use a cacheTime constant and added tests for command aliasing and options ordering. |
| vscode-dotnet-runtime-library/src/LocalMemoryCacheSingleton.ts | Introduced command aliasing functionality and improved cache key serialization with sorted options. |
| vscode-dotnet-runtime-library/src/EventStream/EventStreamEvents.ts | Added a new event class (CacheAliasCreated) to notify when aliases are created. |
| vscode-dotnet-runtime-library/src/Acquisition/DotnetPathFinder.ts | Alias caching is now preloaded when a true dotnet path is determined on Windows. |
| vscode-dotnet-runtime-library/src/Acquisition/DotnetConditionValidator.ts | Added a helper method for constructing runtime commands and improved environment variable handling. |
Comments suppressed due to low confidence (2)
vscode-dotnet-runtime-library/src/LocalMemoryCacheSingleton.ts:72
- Modifying the key object directly in getCommand (and similarly in put) may introduce side effects if the same object is reused later. Consider creating a shallow copy or deriving a new key to avoid unintended mutations.
if (this.commandRootAliases.has(key.command.commandRoot)) {
vscode-dotnet-runtime-library/src/LocalMemoryCacheSingleton.ts:131
- [nitpick] Building the JSON string manually by iterating over sorted keys can lead to issues such as inadvertent trailing commas when keys are skipped; consider accumulating key-value pairs in an array and then joining them with commas for clarity and reliability.
private cacheableCommandToKey(key: CacheableCommand): string
| // /usr/local/bin/dotnet becomes /snap/dotnet-sdk/current/dotnet in reality, may have different behavior in shells. | ||
| if (os.platform() === 'win32') | ||
| { | ||
| LocalMemoryCacheSingleton.getInstance().aliasCommandAsAnotherCommandRoot(`"${truePath}"`, `"${tentativePath}"`, this.workerContext.eventStream); |
There was a problem hiding this comment.
The aliasCommandAsAnotherCommandRoot method is called with quoted string parameters in DotnetPathFinder, while tests and other usages expect unquoted values. This discrepancy can result in mismatches when retrieving cached commands; please ensure consistent string formatting.
Suggested change
| LocalMemoryCacheSingleton.getInstance().aliasCommandAsAnotherCommandRoot(`"${truePath}"`, `"${tentativePath}"`, this.workerContext.eventStream); | |
| LocalMemoryCacheSingleton.getInstance().aliasCommandAsAnotherCommandRoot(truePath, tentativePath, this.workerContext.eventStream); |
MiYanni
approved these changes
Jun 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2 Things.
We resolve the full PATH of 'dotnet'. Then when we know the full path, we call
--list-runtimesagain on the full path.We know the full path is the same as 'dotnet'. So we should alias that in the cache and not redo the work again.
The options object was unsorted by json.stringify and nondeterministic in the JS spec. That would prevent things from being cached correctly. Not so good.
This fixes those things.