Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure createProgram stops referencing rootNamesOrOptions to ensure oldProgram is GC'd #61034

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jan 23, 2025

This is practically the same as #58138, since that seems to be the only thing that works, along with some code to make createProgram less likely to accidentally be changed such that this hack breaks something.

This is still 100% for sure a JSC GC bug, but I cannot find any reasonable way to fix this besides the original proposed solution.

The parameter rootNamesOrOptions holds a reference to oldProgram; we force overwrote oldProgram in the local scope, but rootNamesOrOptions appears to stay in scope in JSC, so overwrite that too. That and createProgramOptions also referenced the variable, so that has to also go.

Fixes #58137

@jakebailey

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@tmm1
Copy link

tmm1 commented Jan 23, 2025

Thank you!!

I also tried several other variations and this is the only thing that seems to work. Clearly a JSC bug, but it is severe enough that a workaround is warranted.

src/compiler/program.ts Outdated Show resolved Hide resolved
@jakebailey
Copy link
Member Author

Hmmm, actually, I have an idea...

@jakebailey
Copy link
Member Author

Aha, I found a more explainable fix.

@jakebailey jakebailey changed the title Ensure oldProgram is released in synchronizeHostDataWorker to work around JSC bug Ensure createProgram stops referencing rootNamesOrOptions to ensure oldProgram is GC'd Jan 23, 2025
@jakebailey
Copy link
Member Author

@typescript-bot pack this

This fix is way more explainable. I only realized it after I sent the PR.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 23, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 23, 2025

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/164619/artifacts?artifactName=tgz&fileId=CD7BECBB0B9227A85A09E7CA9B0974942D47ABEF76E46CBA3A32CB1E3A825A5702&fileName=/typescript-5.8.0-insiders.20250123.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@jakebailey
Copy link
Member Author

@typescript-bot perf test tsserver-only

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 23, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
perf test tsserver-only ✅ Started 👀 Results

@tmm1
Copy link

tmm1 commented Jan 23, 2025

Wow nice find.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,320ms (± 0.45%) 2,333ms (± 0.83%) ~ 2,305ms 2,355ms p=0.297 n=6
Req 2 - geterr 5,308ms (± 0.49%) 5,323ms (± 0.27%) ~ 5,307ms 5,346ms p=0.378 n=6
Req 3 - references 262ms (± 0.45%) 264ms (± 1.48%) ~ 261ms 269ms p=0.680 n=6
Req 4 - navto 227ms (± 0.66%) 224ms (± 0.23%) -2ms (- 1.03%) 224ms 225ms p=0.006 n=6
Req 5 - completionInfo count 1,357 1,357 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 79ms (± 8.89%) 84ms (±10.42%) ~ 76ms 92ms p=0.357 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen 2,437ms (± 0.99%) 2,433ms (± 0.65%) ~ 2,412ms 2,453ms p=1.000 n=6
Req 2 - geterr 4,029ms (± 0.57%) 4,022ms (± 0.62%) ~ 3,997ms 4,055ms p=0.748 n=6
Req 3 - references 279ms (± 0.98%) 280ms (± 1.00%) ~ 274ms 282ms p=1.000 n=6
Req 4 - navto 227ms (± 0.18%) 227ms (± 0.18%) ~ 226ms 227ms p=1.000 n=6
Req 5 - completionInfo count 1,519 1,519 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 83ms (± 5.73%) 85ms (± 5.00%) ~ 76ms 87ms p=0.681 n=6
xstate-main-1-tsserver - node (v18.15.0, x64)
Req 1 - updateOpen 5,276ms (± 0.22%) 5,278ms (± 0.29%) ~ 5,258ms 5,293ms p=0.936 n=6
Req 2 - geterr 1,153ms (± 1.26%) 1,154ms (± 1.76%) ~ 1,135ms 1,188ms p=0.810 n=6
Req 3 - references 76ms (± 0.68%) 75ms -1ms (- 0.88%) ~ ~ p=0.025 n=6
Req 4 - navto 445ms (± 0.81%) 445ms (± 1.20%) ~ 434ms 448ms p=1.000 n=6
Req 5 - completionInfo count 3,462 3,462 ~ ~ ~ p=1.000 n=6
Req 5 - completionInfo 834ms (± 0.96%) 840ms (± 1.58%) ~ 827ms 859ms p=0.688 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • CompilerTSServer - node (v18.15.0, x64)
  • Compiler-UnionsTSServer - node (v18.15.0, x64)
  • xstate-main-1-tsserver - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@jakebailey jakebailey merged commit 51d9a98 into microsoft:main Jan 23, 2025
32 checks passed
@jakebailey jakebailey deleted the fix-58137 branch January 23, 2025 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Backlog Bug PRs that fix a backlog bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Memory Leak disposing oldProgram
5 participants