Trampolines for large binary expressions #36248
Merged
Conversation
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
|
@typescript-bot perf test this - I think my trampoline in the emitter is low/no overhead (and some of the smaller changes to enable it may actually lower stack/memory usage in normal scenarios), and the checker one isn't on a particularly hot path, so I don't expect much, if any, changes to check or emit time (and need to validate that). However the restructuring in the binder likely has performance implications; I am unsure of how much, so definitely need to check it out. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 2c34061. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..36248
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Yeah, OK Now I wanna know what aspect makes it so slow. I should bump up the target to see how bad it is when the generators aren't downleveled. In most tests the |
weswigham
force-pushed
the
trampolines-for-large-binary-expressions
branch
2 times, most recently
from
82f9f2f to
5ec69bf
Compare
weswigham
force-pushed
the
trampolines-for-large-binary-expressions
branch
from
5ec69bf to
6e1c80d
Compare
|
@typescript-bot perf test this now with 100% more runtime provided generators (do note: I'm still manually forwarding the generators in the binder because for some reason that's faster than using |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 6e1c80d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..36248
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Well, +130% is still horrible, but nowhere near as bad. Guess I'm really going to have to write trampolines the hard way, like I did in the emitter :( |
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at c3046b1. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..36248
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There we go, much more reasonable numbers - variations between -2% to +5% on very small numbers. Much better. @rbuckton do you have opinions on this PR in this state? |
|
@typescript-bot perf test this again - now with less (unused) code in the parser (which maybe accounts for the parser time changes). |
|
@weswigham Here they are:Comparison Report - master..36248
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
suggested changes
Jan 18, 2020
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 3295016. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..36248
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@rbuckton perf looks good and your changes are in - anything else to add? |
5 tasks
rbuckton
suggested changes
Jan 22, 2020
|
@rbuckton added the check back in (and fixed the comments) |
rbuckton
approved these changes
Jan 24, 2020
|
Using |
|
It only fully removes the crash (there are many phases which could crash) with the caveats that either |
|
@weswigham hmmmmmm |
|
Here's a modified repro so node prints out the full stack: |
|
@weswigham argh, only repros for JS files because of |
|
welp, that's a new callstack to me, XD The |
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.
Fixes #35633
This introduces trampolines into the binder, checker, and emitter. The binder trampoline is complete - every
bindcall returns to the root of the binder stack to be performed. The checker trampoline is partial - all immediately nested binary expressions are included within a single stack frame ofcheckBinaryExpression. Control flow is much less likely to need a trampoline (and does not in either given example), as the control flow graph is much simplified compared to the AST. However it could be added, if there is interest in doing so. The emitter trampoline is both partial and conditional; our printer handler API makes it awkward to try to trampoline through it, so to avoid breaking it, we only perform an emit trampoline for binary expressions when a node does not need notifications, is not substituted, does not need comment emit, and does not need sourcemap emit. This is quite limiting, but the primary concern here was typechecking and language server operations, so I figure having a way to emit a file like those provided (withremoveComments: true) without crashing might be better than always crashing during emit on them, on the offchance someone includes something like them in a project they're actually emitting. Do note though that we do still just.... run out of stack space, should those conditions not be met. If we're ever comfortable breaking our printer/transformer API, making it trampoline-able (either by return convention, similar to the trampoline in the emitter, or by generator construction, as in the binder) would be something we should do.In addition, a few small other bits got trampolines. In the parser,
fixupParentPointersbecame a pretty trivial iterative algorithm, rather than the recursive one (this is only used during testing, so doesn't appear in the production stacktrace, but is required nonetheless). In the test harness,visitNodein the type/symbol writer now just utilizes a straight iterator of every AST node (which is built utilizing avisitEachChildgenerator-based variant produced to more easily implement implement the binder trampoline) without relying on any recursion.