-
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
[DO NOT REVIEW] Perf testing module-ified TypeScript compiler #50765
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a28e572
to
ecd8076
Compare
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.
677505a
to
5730be9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 6f5ba94. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..50765
System
Hosts
Scenarios
TSServerComparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
That's very good, but I even forgot to disable let and const... So maybe it'll be even faster? |
No, unfortunately esbuild doesn't support lowering let/const. I only though it did because I saw But, it's still a lot faster, even without removing let/const (for inner scopes, top levels become Maybe if I run it on TS's output files, rather than its inputs? (Though, I'd prefer to emit ESNext at that point or something, as to not double-downlevel and add too many helpers) |
@typescript-bot perf test this This tests downleveling to node10 syntax + no object spread (as it's faster to use esbuild's helper). |
Heya @jakebailey, I've started to run the perf test suite on this PR at c86b59c. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..50765
System
Hosts
Scenarios
TSServerComparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this One last time to see if we get any boost from downleveling let/const via tsc. I'm guessing the loss of scope lifting will make this one very bad. After, I'll test the perf of plain modules, no bundling, since I now know the minimum build required for the perf benchmarks. |
Heya @jakebailey, I've started to run the perf test suite on this PR at d2352b2. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..50765
System
Hosts
Scenarios
TSServerComparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
Wow, yeah, running on the emitted output is very bad. |
@jakebailey this is very interesting work! I'm very curious about what is exactly going on here and what exact approaches you have been testing etc. Would you mind sharing what strategy was used by this perf run? Are the main gains here from ditching the property lookups involving |
I'd recommend looking at #49332, https://github.com/jakebailey/typeformer, and the stack starting at jakebailey#1 (though not up to date yet). Effectively, there's a big transform that converts the codebase from namespaces into modules, using "barrel" files to export what used to be in those namespaces, so consumers see the same API. Since these are all regular imports, a bundler like esbuild can transform the code as needed, and so in this case, all of the code turns out to be brought to the top level, so no indirection, and then the big barrels are retained as objects that are exported. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I screwed up the server build for this case (the correct path to tsserver is |
93f00e4
to
a3ca90b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a3ca90b
to
1f5d83c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1f5d83c
to
6e9d39a
Compare
@typescript-bot perf test this Retry CJS again... I forgot to add my changes 😢 |
Heya @jakebailey, I've started to run the perf test suite on this PR at 6e9d39a. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..50765
System
Hosts
Scenarios
TSServerComparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this This time, for a hacky Node16 emit. |
This comment was marked as outdated.
This comment was marked as outdated.
@typescript-bot perf test this The perf runner wasn't able to build things, I have no idea why. |
Heya @jakebailey, I've started to run the perf test suite on this PR at 68fd1a7. You can monitor the build here. |
@typescript-bot perf test this faster Node 10 and 12 don't seem to work, for unrelated reasons. |
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 68fd1a7. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:Comparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
Nice; ESM specifically gives us a 3-9% speedup, which should correspond to the benefit of directly accessing functions (as they have been imported into each module's scope, versus being fully qualified). |
@typescript-bot perf test this faster One last test; this time esbuilding the emitted ESM from tsc. |
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at c5bf532. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:Comparison Report - main..50765
System
Hosts
Scenarios
Developer Information: |
I'm going to close this PR; the patches that I used for this have been saved in https://github.com/jakebailey/typeformer/tree/master/old/patches-after-perftesting, but I'd like to continue working (and I don't want to continue to spam this PR with my force pushes as I continue to rebase my stack). |
See #49332 for more info.