-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Experiment - monomorphic node/type/signature #58928
base: main
Are you sure you want to change the base?
Experiment - monomorphic node/type/signature #58928
Conversation
@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: |
This is the most green thing I've seen this week. Nice!! |
Unfortunately I didn't see a noticeable perf improvement in the Airtable code base, but did see a 5-10% increase in peak memory usage. |
@MichaelMitchell-at i find this a bit surprising. While the % of difference varies, we've generally noticed improvements across the board. Is this a public repo? If it not, could you run the tsc on this branch through dexnode to get a report for the inline caches? Also how did you measure the times? What is the output with - - diagnostics? |
Actually there seems to be an error with my methodology so throw out the previous result I mentioned. I can't seem to apply the patch cleanly at the moment. I'll just have to wait for this to reach nightly. |
Ok, I looked into it a bit more. Our custom runner uses the compiler API and that code path hasn't been updated to support the new node structure yet.
I should be able to get past this if I can swap out this part with a lib that can parse JSON with comments and trailing commas. Edit: |
49b3d87
to
977cbc5
Compare
@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: |
Latest perf results seemed to have clawed back most of the parser perf loss. |
Half a gig of memory for vscode is a bit scary, though, given how close that is to the max memory limit... |
…node to reduce memory usage.
@jakebailey I did some extra work. I was too eager to make everything monomorphic, a bit of polymorphism is fine. There should be 3 shapes for nodes now: Token, Identifier and Node. There is a 4th one Token with With these changes, at least locally the increase in mem usage goes form 13% to about 5%, while keeping the same performance profile. |
@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: |
Much better, though emit is now much slower on certain benchmarks. Maybe a case or two aren't monomorphic for those benchmarks? |
For the extra parse time I have a pretty good idea. Will look into it a bit more. |
I moved some stuff around between node and node data. This seems to reduce some more of the extra memory that was being used. I also experimented with reducing the size of Types, but don't think there are any easy wins there. @jakebailey When you get a chance, can you run the tests again please? |
@typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@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: |
Good stuff. On Node the mean averages for
...which is an exchange rate of 4.2 |
It's worth keeping in mind that if TSServer requires more than 4096 MB of memory then VSCode can no longer use its prebuilt version of Node with pointer compression, which has a significant effect on interactivity performance. So it might be worth re-measuring that exchange rate on projects that are close to reaching that threshold. I plan to do this on the Airtable monorepo (which at this point has already blown past the 4GB memory limit, but we hope to eventually bring it down), once the PR is stable. |
@jakebailey Can you please run the perf tests one more time? I hope this latest version will bring the mem usage a bit more, either way I think this is at the limit of what I can do to reduce memory usage. I can cleanup the PR if there is interest in merging this based on these perf metrics. Beside performance, we should also consider the issue of this being a breaking change for API consumers. Anyone manually iterating over the properties of nodes (either with Possible future improvements: Actually use |
I'll rerun it, though I do think there are still concerns about this particular approach. @typescript-bot perf test this |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@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: |
Wow that's even better. On Node the mean averages for
...which is an exchange rate of 5.8 on total time, and 7.6 on checker time. |
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Conclusions from breaking down where the performance win comes from: AST Nodes: Types: Signatures: The AST nodes win is smaller than I remember in the first experiments. This is probably due to the reduction in memory which meant some relatively common properties are now in |
get sourceText() { | ||
return this.data?.sourceText; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might look a bit silly, but if any of these accessors are a particular hot path, you can make an accessor monomorphic by having a switch/case on this.kind
and having several branches that all read e.g. this.data.sourceText
but behind different case
headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that. It did not improve performance in my experiment. And I tried several different versions.. switch, if - linear search, if-binary search. All performed worse.
Experiment to make several objects monomorphic by keeping the common properties in a single object shape and moving all other properties to a separate object and using accessors to preserve compatibility with existing code.
Local improvements seem promising, building TSC/compiler: -20% in time with +10% in memory(See bellow for results)Code is rough around the edges, just testing out as a proof of concept.
Note: This is the result of me and @acutmore bouncing ideas about improving access to
Node.kind
Update
On Node, the improvements with everything in this PR are:
Which is an exchange rate of 5.8 on total time, and 7.6 on checker time.
Breaking down where the performance win comes from (from the separate PRs):
AST Node monomorphism:
Types monomorphism:
Signatures monomorphism:
The AST nodes win is smaller than I remember in the first experiments. This is probably due to the reduction in memory which meant some relatively common properties are now in
data
not inNode
.