-
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
Add separate PrivateIdentifier and SourceFile constructors #36844
Conversation
@typescript-bot perf test this |
Heya @sandersn, I've started to run the perf test suite on this PR at 7228ef0. You can monitor the build here. Update: The results are in! |
PrivateIdentifiers are rare and there are none in |
@sandersn Here they are:Comparison Report - master..36844
System
Hosts
Scenarios
|
Actually the gains are from |
I'd like @rbuckton to sign off on this before merging. When I showed it to him yesterday, he pointed out that it's probably overkill to give all Nodes their own constructors, but the top 10 would benefit. Things like BinaryExpression, PropertyAccessExpression, maybe ElementAccessExpression. I'll let him give the entire list. |
I also don’t think giving each node its own constructor is worth it. At least not now with the way that the API is structured. |
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 7228ef0. You can monitor the build here. |
I'll merge this in if the numbers look reasonable, otherwise there's no use in stalling this for generalizing it to other node types. |
I think this needs a rebase at least. |
7228ef0
to
458e665
Compare
@DanielRosenwasser rebased, please try again |
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 458e665. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:Comparison Report - master..36844
System
Hosts
Scenarios
|
This lowers the memory usage when compiling `src/compiler` from **277M** to **273M**, so a modest ~1.5% win.
458e665
to
99b9eee
Compare
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 99b9eee. You can monitor the build here. Update: The results are in! |
@rbuckton Here they are:Comparison Report - main..36844
System
Hosts
Scenarios
Developer Information: |
This doesn't seem to have a consistent positive effect on compilation as memory usage and compilation time are mostly unchanged. I don't think this change is warranted at this time. |
This lowers the memory usage when compiling
src/compiler
from 277M to 273M, so a modest ~1.5% win.
Same as #33431, this further splits
Node
fromSourceFile
, which reduces the size ofNode
s from 160 bytes to 144