-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Monomorphize allocators for tsserver/public API, just like core compiler #58045
Conversation
@typescript-bot perf test this |
After the results come back, I'll try and rebase this PR such that it targets a branch that has the tsc change in place. The benchmark system is smart enough to compare a PR against its target branch so that will provide better results than comparing two negative results to see which one is less negative. (I am liking what I'm seeing in the logs so far, though.) |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
14249be
to
a301800
Compare
@typescript-bot perf test this |
Those results are a lot better than #57703 (comment). |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Yessss |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
FWIW comparing #51682 (comment), this increase memory is not unexpected. Maybe it's fine; this benchmark is already extra strange because I'm emitting stuff, which won't happen in tsserver... normally anyway. I feel like if #51682 (comment) was "good", then this PR is good. |
@typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
The |
tests/baselines/reference/skipJSDocParsing/deprecated-ParseNone-file.ts.diff
Show resolved
Hide resolved
tests/baselines/reference/skipJSDocParsing/see-ParseForTypeInfo-file.ts.diff
Show resolved
Hide resolved
cd378b4
to
af103bc
Compare
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
I've rebased and retargeted the PR to main. Rerunning benchmarks to check without forcing tsc's allocators. @typescript-bot perf test this |
@typescript-bot pack this |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
The above looks exactly as expected for this PR targeting main; no change to tsc (it doesn't use this code), and all benefit for tsserver. |
@jakebailey Here are the results of running the top 400 repos comparing Everything looks good! |
The benchmarking system now includes a benchmark for simulating tsc via the public API; testing: @typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
In #51682 and #51880, @rbuckton monomorphized our Nodes and Symbols for the compiler's allocators.
This PR naively copies the property order from the compiler over to the classes we define in
services
, netting similar performance wins for similar memory costs. This should significantly improve performance of tsserver (every editor) and public API users who do anything more than parse (ts-eslint, ts-loader, ts-jest, rush, nx, the list goes on...).Here are the latest benchmarks when forcing
tsc
to use these allocators, which is a good proxy for how this will affect downstream users: #58045 (comment)Note that this is not a performance improvement for
tsc
itself; that benefit already happened in TS 5.0!The tsserver benchmarks remain, and should lead to significant improvements in editor performance:
A new benchmark tests "tsc via public API" explicitly: