startup: Cache packaged ESM ASAR resolutions - #334769
Open
Dmitriy Vasyura (dmitrivMS) wants to merge 2 commits into
Open
startup: Cache packaged ESM ASAR resolutions#334769Dmitriy Vasyura (dmitrivMS) wants to merge 2 commits into
Dmitriy Vasyura (dmitrivMS) wants to merge 2 commits into
Conversation
Reuse successful archive resolutions for identical ESM contexts while preserving first-resolution precedence for closer dependencies. Cover cache hits and parent isolation with a packaged ASAR fixture. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Dmitriy Vasyura (dmitrivMS)
September 6, 2026 07:09
View session
Dmitriy Vasyura (dmitrivMS)
enabled auto-merge (squash)
September 6, 2026 07:13
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The non-injective cache key can incorrectly reuse resolutions across distinct contexts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/bootstrap-esm.ts — The flattened key is not injective: conditions [..., 'type', 'json'] with no attributes produce… |
What changed in this PR
Adds process-local caching for packaged ESM ASAR resolutions.
Changes:
- Caches resolutions by specifier, parent, conditions, and attributes.
- Preserves closer-dependency precedence.
- Adds packaged-ASAR integration coverage.
A critical cache-key collision can return incorrect resolutions; collections need structural encoding and regression coverage. Direct fallback caching for packages without exports also lacks coverage.
| File | Description |
|---|---|
src/vs/code/test/node/bootstrapESM.test.ts |
Adds cache reuse and parent-isolation tests. |
src/bootstrap-esm.ts |
Implements caching; requires collision-safe keys and fallback-path coverage. |
Suppressed comments (1)
src/bootstrap-esm.ts:196
- The new test package declares
exports, so it only exercises the self-reference cache write above; this direct fallback cache path for packages without exports remains untested. Add a second archived package withoutexportsand assert that repeated imports produce one archive lookup followed by a cache hit.
const result = { url, shortCircuit: true };
resolutionCache.set(cacheKey, result);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Encode conditions and import attributes as separate structural tuple fields and cover the boundary collision with conditional exports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Sep 6, 2026
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.

Summary
node_modules.asarThe cache is process-local and lives for the process lifetime, so it also helps repeated dynamic imports after startup. CommonJS, built-ins, Electron modules, external extension paths, closer dependencies, and failed resolutions are unchanged.
Performance
The motivating main-process trace found 33 ASAR resolution attempts from
out/main.jsfor seven unique specifier-parent pairs, including 15 resolutions of@vscode/spdlogand eight of@vscode/policy-watcher.Packaged resolver benchmark
40 alternating baseline/cached Electron runs, excluding five warmups, shaped as 33 import sites over seven unique contexts:
Main-process CPU profiles
Six packaged launches per variant, excluding one warmup:
internalModuleStatmedianThe profiler adds roughly 33-35% overhead in this local Code OSS package, so the CPU deltas are more reliable than the profiled wall-time delta.
Full packaged startup
Ten unprofiled launches per variant, excluding one warmup:
The median wall-time change is below run variance, so this PR does not claim a statistically significant end-to-end startup improvement. The demonstrated benefit is reduced resolver and main-process CPU, including for later repeated imports.
For the full-package A/B, the baseline forced the generated cache lookup to miss while retaining the small key/map overhead; every other application byte was identical. The standalone resolver benchmark compares the exact pre-change and changed bootstrap revisions.
Validation
npm run typecheck-clientvscode-win32-x64-minpackage buildgit diff --check