Reduce polymorphism resulting from unstable Node shapes #51682
Merged
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
typescript-bot
added
Author: Team
For Uncommitted Bug
rbuckton
force-pushed
the
less-polymorphic
branch
from
432485f
to
70cdafc
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 70cdafc. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..51682
System
Hosts
Scenarios
TSServerComparison Report - main..51682
System
Hosts
Scenarios
StartupComparison Report - main..51682
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Andarist
reviewed
Nov 30, 2022
| @@ -84,3 +84,4 @@ tests/cases/user/puppeteer/puppeteer | |||
| tests/cases/user/axios-src/axios-src | |||
| tests/cases/user/prettier/prettier | |||
| .eslintcache | |||
| *v8.log | |||
There was a problem hiding this comment.
a question, if I may, what kind of tools you were using to investigate those things? I imagine that in the case of Nodes the issue was already known for some time but I wonder how I could test those things in my libraries.
There was a problem hiding this comment.
I have an internal tool I wrote to analyze log files generated by various V8 commandline options.
There was a problem hiding this comment.
@Andarist One open source tool that might be of interest is https://github.com/thlorenz/deoptigate
rbuckton
requested review from
jakebailey,
ahejlsberg,
sandersn and
weswigham
sandersn
requested changes
Dec 5, 2022
rbuckton
force-pushed
the
less-polymorphic
branch
3 times, most recently
from
e1ab7ed
to
f476065
Compare
|
@typescript-bot perf test Running again after rebasing against |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at f476065. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..51682
System
Hosts
Scenarios
TSServerComparison Report - main..51682
System
Hosts
Scenarios
StartupComparison Report - main..51682
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 5f84fc6. You can monitor the build here. Update: The results are in! |
rbuckton
force-pushed
the
less-polymorphic
branch
from
5f84fc6
to
cad5e7b
Compare
|
@rbuckton Here they are:
CompilerComparison Report - main..51682
System
Hosts
Scenarios
TSServerComparison Report - main..51682
System
Hosts
Scenarios
StartupComparison Report - main..51682
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
weswigham
reviewed
Dec 7, 2022
| @@ -2803,18 +2738,20 @@ function createBinder(): (file: SourceFile, options: CompilerOptions) => void { | |||
| } | |||
| // falls through | |||
| case SyntaxKind.ThisKeyword: | |||
| // TODO: Why use `isExpression` here? both Identifier and ThisKeyword are expressions. | |||
There was a problem hiding this comment.
IIRC this in a type position wasn't an expression, but was still parsed as a ThisKeyword for a period of time.
weswigham
approved these changes
Dec 7, 2022
|
@Jack-Works I'm not sure how closely this relates to the stats posted by the bot here (most of them overlap but I'm not sure if they are 1 to 1) but you can call |
The tool we use for benchmarking isn't public, but it essentially just runs I'm planning to publish the tool I wrote to analyze V8 maps and deoptimizations in the near future, however. |
|
System info: Node 19.2.0, Windows 10.0.22623, AMD64 Family 25 Model 33 Stepping 0 AuthenticAMD ~3701 Mhz Project scale: Perf result: 5.0.0-dev.20221207 => npm:@typescript-deploys/[email protected] |
sandersn
approved these changes
Dec 10, 2022
rbuckton
force-pushed
the
less-polymorphic
branch
from
cad5e7b
to
684bfda
Compare
jakebailey
reviewed
Dec 13, 2022
src/compiler/emitter.ts
Outdated
| @@ -28,7 +28,7 @@ import { | |||
| BundleFileTextLikeKind, | |||
| CallExpression, | |||
| CallSignatureDeclaration, | |||
| CaseBlock, | |||
| canHaveLocals, CaseBlock, | |||
There was a problem hiding this comment.
There's no lint rule for this, but these should be columns.
Hopefully one day we can get formatting automated...
There was a problem hiding this comment.
One day...
🤣 (sorry, I couldn't resist)
There was a problem hiding this comment.
There's no lint rule for this, but these should be columns.
Hopefully one day we can get formatting automated...
I've also noticed that Organize Imports seems to be sorting __String to the end rather than the start.
rbuckton
force-pushed
the
less-polymorphic
branch
from
684bfda
to
6bfe986
Compare
rbuckton
force-pushed
the
less-polymorphic
branch
from
6bfe986
to
afc0496
Compare
11 tasks
devanshj
added a commit
to devanshj/TypeScript
that referenced
this pull request
Jan 3, 2023
9 tasks
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.
We have a number of places in our codebase where we optionally attach properties to a
Node. While a common practice in JS, this can sometimes have negative consequences with regards to runtime performance. Most modern JavaScript engines use Inline Caches (ICs) to optimize code paths, primarily when reading or writing to properties on objects, or to global variables.Ideally, we want property ICs to be "monomorphic", such that they only ever see a single object shape (or "map" in V8 parlance) as "monomorphic" property ICs provide the fastest lookups. A "polymorphic" IC has slightly worse performance since it shifts from a one-to-one lookup to a short list of "map"-> property entries. If a new "map" is encountered once a polymorphic IC has around four entries, V8 will shift to a "megamporphic" IC. "Megamorphic" property ICs are the worst in terms of performance, as V8 essentially gives up on fast property access and relies on slow property lookups (there is still a cache, but it is cyclical: repeated property lookups for the same "map" will be fast but will age out as new "maps" are encountered).
Monomorphism isn't always possible, especially when accessing properties like
.kind. However, in cases where we have branched on.kind, we should endeavor to ensure that further property accesses within that branch remain monomorphic, or at least polymorphic.One way to achieve this is to ensure the shape of every
Nodeis stable relative to itskind. This has the following implications:Nodeand its subtypes really shouldn't be "optional" and should be fully initialized when aNodeis produced (even if only withundefined).node as any as JSDocContainerandnode as TypeNode & { <optional properties> }).symbol,localSymbol,locals,nextContainer,flowNode, etc.), as that results in uncheckednode.symbolaccesses which often result in "wrong map" deoptimizations.kindwhen the node is a supertype or union, unless the function is only accessing one or two properties (likepos/end) since a megamorphic lookup on "pos" will have the same cost as a megamorpic lookup on "kind".Nodesubtype, especially if it accesses multiple properties.There is a trade-off for these changes, however. Pre-defining potentially unused properties on a
Nodeincreases the memory footprint. As such, if the memory cost becomes too severe, we may want to investigate removing properties from someNodesubtypes where the value can be trivially recomputed from source or moved into a cache likeNodeLinksorEmitNode.