Optimize layer checker memory use - #327953
Draft
roblourens wants to merge 2 commits into
Draft
Conversation
(Written by Copilot) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Screenshot ChangesBase: Changed (6)
|
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (panel)/Light
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (wide)/Dark
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (wide)/Light
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (narrow)/Dark
| Before | After |
|---|---|
 |
 |
agentsVoice/voiceModeOnboarding/Voice Mode onboarding (narrow)/Light
| Before | After |
|---|---|
 |
 |
1 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Reduces layersChecker memory usage by limiting semantic symbol resolution, excluding skipped roots from TS program creation, and matching rules relative to the configured source root, with new focused tests.
Changes:
- Refactors
layersCheckerto collect violations programmatically (checkProgram,runLayerChecker) and to use relative-path rule matching (getRule) - Filters skipped rule roots out of
ts.Programcreation to reduce work and peak memory - Adds node:test-based coverage for rule matching, skipped roots, aliased forbidden types, and inferred forbidden-type member detection
Show a summary per file
| File | Description |
|---|---|
| build/checker/layersChecker.ts | Refactors checker to use relative-path rule selection, reduce symbol resolution work, exclude skipped roots from the program, and centralize violation reporting. |
| build/lib/test/layersChecker.test.ts | Adds targeted tests for the new rule matching, program root filtering, and forbidden-type detection behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+117
to
141
| function checkFile(checker: ts.TypeChecker, sourceFile: ts.SourceFile, rule: IRule, violations: ILayerViolation[]): void { | ||
| const disallowedTypes = new Set(rule.disallowedTypes); | ||
| const candidateNames = new Set(disallowedTypes); | ||
|
|
||
| collectAliases(sourceFile); | ||
| checkNode(sourceFile); | ||
|
|
||
| function collectAliases(node: ts.Node): void { | ||
| if ((ts.isImportSpecifier(node) || ts.isExportSpecifier(node)) && disallowedTypes.has((node.propertyName ?? node.name).text)) { | ||
| candidateNames.add(node.name.text); | ||
| } | ||
|
|
||
| ts.forEachChild(node, collectAliases); | ||
| } | ||
|
|
||
| function checkNode(node: ts.Node): void { | ||
| if (node.kind !== ts.SyntaxKind.Identifier) { | ||
| if (!ts.isIdentifier(node)) { | ||
| return ts.forEachChild(node, checkNode); // recurse down | ||
| } | ||
|
|
||
| const checker = program.getTypeChecker(); | ||
| if (!candidateNames.has(node.text) && !(ts.isPropertyAccessExpression(node.parent) && node.parent.name === node)) { | ||
| return; | ||
| } | ||
|
|
||
| const symbol = checker.getSymbolAtLocation(node); |
Comment on lines
+233
to
+235
| for (const violation of violations) { | ||
| console.log(`[build/checker/layersChecker.ts]: Reference to type '${violation.type}' violates layer '${violation.target}' (${violation.fileName} (${violation.line},${violation.character}). Learn more about our source code organization at https://github.com/microsoft/vscode/wiki/Source-Code-Organization.`); | ||
| } |
(Written by Copilot) Co-authored-by: Copilot <223556219+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.
Summary
Reduce the custom layer checker's memory usage while preserving and strengthening the checks it performs.
The immediate heap-limit fix landed in #327942. This follow-up addresses why the checker needs so much memory in the first place:
skiprules from the TypeScript program's root filesThe 8 GB heap limit remains in place as growth headroom.
Why the checker used so much memory
layersChecker.tscreates one TypeScript program for the fullsrc/tsconfig.jsonsource tree. Before this change, it then calledgetSymbolAtLocationfor every identifier in every file matched by a layer rule.On the source tree used during the investigation, that meant:
Calling
getSymbolAtLocationlazily expands and retains TypeScript's semantic graph. Resolving nearly 2.5 million symbols therefore pushed the checker past Node's default ~4 GB old-space limit even though the configured rules only care about a small set of native types andipcMain.This was gradual source-tree growth crossing a fixed threshold, not a single recent TypeScript regression. With fixed dependencies, the checker's peak footprint grew from approximately 3.55 GB in December to over 5 GB in July.
What changed
1. Limit semantic lookups to plausible candidates
The checker now resolves symbols only for:
The property-access case is important. A forbidden type may be inferred without its name appearing in the consuming file:
The checker still resolves
runand follows its declaration container back to the forbidden interface. This preserves detection of inferred forbidden types instead of applying a naive text-only filter.2. Handle aliases explicitly
A preliminary syntax pass records aliases such as:
Both the alias declaration and later
Servicereferences remain candidates. Alias symbols are resolved throughTypeChecker.getAliasedSymbol, so renaming an import or export cannot bypass the rule.3. Follow containing declarations through public APIs
The old implementation relied on TypeScript's private
Symbol.parentfield. The new implementation follows aliases and uses symbol declarations to find containing classes, interfaces, enums, and modules.This is type-safe, uses public compiler APIs, and makes member detection explicit. It also required making two existing exceptions explicit:
ipcMaininternally4. Exclude skipped roots before program creation
Files matched by a
skiprule, currently tests, were previously excluded only from the final AST traversal. They were still root files in the TypeScript program and therefore still parsed and bound.They are now removed from
rootFileNamesbeforecreateProgram. This is behavior-safe because those files were never checked. If a non-skipped source file imports one of them, TypeScript still includes it through normal module resolution; it is only removed as an unconditional root.5. Match source-relative paths
Rules previously matched absolute file names. A checkout under a hidden directory could cause
minimatchto reject every rule because wildcard matching does not consume leading dot-prefixed path segments by default.Rules now match paths relative to the tsconfig directory, making behavior independent of the checkout's parent directories and path separators.
Why this is safe
The optimization preserves all routes by which the current rules can be violated:
getAliasedSymbolipcMainaccessThe six layer-specific
tscinvocations are unchanged and continue to enforce the broader browser, worker, Node.js, and Electron library boundaries.Focused tests cover:
Performance
Measured locally on the same source tree:
This removes roughly 1.2 GB of peak footprint while retaining the 8 GB CI allowance for future growth.
Validation
node --test build/lib/test/layersChecker.test.tsnpm --prefix build test -- --test-name-pattern='layersChecker'(239 tests)npm --prefix build run typechecknode build/eslint.ts build/lib/test/layersChecker.test.ts build/checker/layersChecker.tsnpm run valid-layers-checknpm run precommit(Written by Copilot)