Port pyright optimization that skips loop flow nodes that dont contain relevant references #54460
Conversation
This file contains 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
…n relevant references Fixes microsoft#51525 This requied reworking how we compare references so we could equate them with a `set` - that change, in and of itself, may actually also be perf positive (since we now get to cache the walk over the reference's AST), but both changes together is somewhere between neutral and 4% gains in local testing, depending on the suite. I'll need to see if this bears out on the much slower CI box, though, since 4% locally is < 0.1s, which is well in the realm of "OS scheduler variance" for some of these tests on my box. Do note, unlike pyright, which in the linked issue's change, calculates the references used within a loop during binding, we calculate it late on-demand and cache it in the checker. This is basically required for us, since we equate references using the identity of the resolved symbol of the root node of the reference (which... may be a bit circular with expression checking!).
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at b8a7bbe. You can monitor the build here. Update: The results are in! |
weswigham
commented
May 30, 2023
| } | ||
|
|
||
| /** | ||
| * @returns A cache key that looks like `1234."field"."\"prop\".\"name\""` or `undefined` if the node can't be a reference |
There was a problem hiding this comment.
While this is somewhat resilient against antagonistic field names, the escaping is potentially costly, and I'd like to try to avoid it if this shows promise. A less readable key format, like 1234.23.57 might be better (where 23 and 57 are indexes into a known-field-name-strings holding array). Still, I've cached the results, so it's not like we repeat the work, so savings by making this less readable aren't likely to be huge.
weswigham
commented
May 30, 2023
| let references: ReadonlySet<never> | Set<string> = emptySet; | ||
| if (isStatement(node) || isExpressionNode(node)) { | ||
| // don't descend into variable declarations or binding elements, since those contain identifiers we don't want to collect | ||
| forEachChild(node, visitor); |
There was a problem hiding this comment.
The self-check failure points out that I need to swap this to use forEachChildRecursively (since that trampolines internally), since we allow binary expressions to nest greater than the max stack depth possible to traverse in the normal cached call/worker function used in the checker (because the parser is very stack depth efficient). That probably won't affect performance much.
I knew I'd need to do this, I didn't think we had hugely chained binary expressions in our own codebase, though - very much wondering which expressions are long enough to be an issue.
Can you give an example of where this comes up? Is it for a computed reference like this? const propName = "a";
obj[propName]; |
|
@weswigham Here they are:
CompilerComparison Report - main..54460
System
Hosts
Scenarios
TSServerComparison Report - main..54460
System
Hosts
Scenarios
StartupComparison Report - main..54460
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
More like to support block scoped variable shadowing - let x: 1 | 2 = 1;
x; // narrowed to `1`
{
let x = 2
x; // only the x within this block - `number`
}
x; // still `1`which is probably the core reason it uses symbol identity for the roots and not lookup names, and then gets further complicated when the It's mostly speculation on my part, since that's just how it's always been, at least as far as I've known. We've always compared symbols when looking at the root identifiers of references, and not the identifier text itself. |
|
squints +5% in TFS, -1% in compiler-unions. OK, perf testing on my local machine is useless; results are nothing like the CI box. System noise is a lot worse when the tests run an order of magnitude faster. Guess I'm pushing all my iterations on this. Since there's a small perf improvement to |
|
i was hoping for obvious gains but my memory may be so poor that maybe this only manifests in examples with lots and lots of stuff going on. will test with my own perf machine to verify as this is very weird @typescript-bot perf test faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at b8a7bbe. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:Comparison Report - main..54460
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I haven't gotten any version of this to have an outsized, consistent effect locally on our test suite; I'm going to assume our existing flow loop cache largely subsumes this, making it much less impactful. |
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 #51525
This required reworking how we compare references so we could equate them within a
set- that change, in and of itself, may actually also be perf positive (since we now get to cache the walk over the reference's AST), but both changes together is somewhere between neutral and 4% gains in local testing, depending on the suite. I'll need to see if this bears out on the much slower CI box, though, since 4% locally is < 0.1s, which is well in the realm of "OS scheduler variance" for some of these tests on my box. Do note, unlike pyright, which in the linked issue's change, calculates the references used within a loop during binding, we calculate it late on-demand and cache it in the checker. This is basically required for us, since we equate references using the identity of the resolved symbol of the root node of the reference (which... may be a bit circular with expression checking!). This means we don't get to collect the references for "free" while doing the binder AST walk; but given how often the set of references is referred to by control flow, it should probably still be an improvement in all but pathological cases (eg, thousands of references within a loop, none outside of it).