Improve performance of fs operations in extension host#324070
Open
DavyLandman wants to merge 2 commits into
Open
Improve performance of fs operations in extension host#324070DavyLandman wants to merge 2 commits into
DavyLandman wants to merge 2 commits into
Conversation
The fast path of a "local" fs operation in the extension host would always still invoke a function in the proxy (that would return quickly). But with large amount of small IO, the overhead of the rpc-calls and the await on the result was causing a 3x slowdown.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the extension-host fast-path for vscode.workspace.fs operations by avoiding an IPC $ensureActivation(scheme) roundtrip on every shortcut file-system call once the scheme has already been activated, reducing overhead for workloads that perform many small operations.
Changes:
- Replaces per-operation
$ensureActivationcalls in the shortcut path with a per-scheme activator (provider.activate()). - Introduces
buildActivator(scheme)to memoize activation for a provider entry. - Extends the internal provider map entries to carry the activator function.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Context
We have an extension that was doing some heavy IO operations using the
vscode.workspace.fsinterface (for operations on bothfileand custom virtual filesystems). We noticed it was quite a bit slower than using "raw" nodejsfs/promise(readFile/readDirectory). Important to note that it was many small files in many nested directories.During profiling we noticed more than half of the time spend idling, and this let us to inspect the code of VS Code a bit more.
Analysis
#136657 and #172345 created a fast path such that both
file:///and virtual file systems registered inside the extension host do not need to "cross" the bridge to the rendering process that often. Especially the result of the fs functions did not need to be serialized across the RPC. For large files or large directory listing, this makes quite a big impact.However, before every FS operation, VS Code would make sure that the VFS was registered (even for the
fileprovider) like so:in side the rendering process this would quickly terminate if there is already a provider registered, so for infrequent IO, this small call and the rpc-overhead is fine, it takes some time, like you'll have to wait for one of the next ticks to arrive before it continues, but impact not so bad.
However if you're doing a lot small IO operations (like crawling a directory with many files, and then reading them, or first calling stat on them, etc) this starts to add up.
Proposed solution
This PR introduces a small function that only sends this
ensureActivationthe first time, and that that does not send it anymore. As thedisposeclears the map entry, I do not see why we would need to resend the activation every time.An alternative would be to send the
ensureActivationbut leave the promise floating, so that we don't join the queue. I think that's less nice of a solution, but in a way it's less invasive of a change than this PR proposes.