Fix deprecated contextual property memory regression - #4825
Fix deprecated contextual property memory regression#4825Jake Bailey (jakebailey) wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Defers contextual deprecation checks until suggestion diagnostics to prevent speculative overload checks from ballooning memory.
Changes:
- Tracks contextual object/JSX nodes for deferred checking.
- Deduplicates deprecation suggestions by location and code.
- Adds regression coverage for overloaded contextual properties.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/checker/checker.go |
Implements deferred checks and diagnostic deduplication. |
internal/checker/jsx.go |
Registers JSX attributes for deferred checks. |
internal/checker/types.go |
Adds per-source-file deferred-check state. |
internal/fourslash/tests/deprecatedContextualPropertyOverload_test.go |
Verifies deprecation diagnostics with overloads. |
|
tried this branch against the same large private repo that replicated in #4824 -- seems to fix the ram regression entirely, seeing about the same ram use as baseline on main. |
|
Can't we avoid a new deferred check mechanism here by just reusing contextualType := c.getApparentTypeOfContextualType(node, ContextFlagsNone)
for _, property := range node.Properties() {
if property.Name() != nil && !ast.IsComputedPropertyName(property.Name()) {
c.checkDeprecatedProperty(property.Name(), contextualType)
}
}(which would also fix some stuff in this PR RE non-identifier props it happens to break and make the contextual type lookup consistent between JSX and object literals (it should always be apparent since looking for members inside |
|
That does work, though does use a bit more memory. TypeScript Bot (@typescript-bot) perf test this |
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
lspComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
Hm, so we're spending more memory just to track dupe suggestions upfront, only to dedupe them later (why is why the issue was partially us keeping a lot of copies around but only during the build); any reason we're not deduping all diagnostic collections on .Add? We always do a SortAndDeduplicate in collectCheckerDiagnostics as-is when we could just be sorting/deduping upfront, no? The lazy sort-on-demand in the diagnostic collection and then sort-again-and-also-dedupe-in-program we're doing right now is sorta a weird division of labor, right? Surely maintaining a sorted/deduped list upfront is better at this point?
|
Part of the problem with the checker diags is that we add them and then modify them, which makes deduping sort of annoying. We could fix that, though. |
|
I took a stab at it. PTAL |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/checker/checker.go:2536
- The new deferred kinds permanently grow each source file's
deferredNodesbacking storage.checkDeferredNodesdrains the set withClear, butcollections.OrderedSet.Clearexplicitly retains both the key slice and map allocation (internal/collections/ordered_set.go:38-41,ordered_map.go:181-187). BecauseSourceFileLinkslives for the checker lifetime, registering every object literal and JSX attribute leaves capacity proportional to all such nodes resident after checking, undermining this PR's memory-regression fix. Release/reinitialize the set after the one-shot deferred pass rather than retaining its storage.
| key := getDiagnosticLocationKey(diagnostic) | ||
| if existing := c.diagnosticIndex[key]; existing != nil { | ||
| if EqualDiagnostics(existing, diagnostic) { |
Fixes #4824
A bit nasty to prevent memory balooning from deferring stuff, plus code to quickly discard duplicate diags for deprecations.