Monomorphic Symbol access #51880
Merged
Monomorphic Symbol access #51880
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
|
Need to wait on the benchmark CI to catch up to #51682 before I schedule a perf test run. |
rbuckton
force-pushed
the
monomorphic-symbol
branch
from
78c4a11 to
4d64583
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 4d64583. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..51880
System
Hosts
Scenarios
TSServerComparison Report - main..51880
System
Hosts
Scenarios
StartupComparison Report - main..51880
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
|
It seems we have a few unchecked casts to |
|
If one of the results is zero, the compiler crashed somewhere. (I hit this back when I broke the parser but only in some cases.) (We should make that more obvious, we just need to have it distinguish error codes somehow.) |
|
Thats why I commented:
Even after I fixed the crash locally though I have a few failing tests for "infer from usage" code fixes that I have to run down. |
|
Ah, I thought it was just unrelated commentary on the PR, oops. |
|
Apparently, we also have unchecked casts to |
rbuckton
force-pushed
the
monomorphic-symbol
branch
from
4d64583 to
d099445
Compare
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at d099445. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..51880
System
Hosts
Scenarios
TSServerComparison Report - main..51880
System
Hosts
Scenarios
StartupComparison Report - main..51880
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
force-pushed
the
monomorphic-symbol
branch
from
d099445 to
6a5956f
Compare
rbuckton
commented
Dec 14, 2022
| /** @internal */ assignmentDeclarationMembers?: Map<number, Declaration>; // detected late-bound assignment declarations associated with the symbol | ||
| } | ||
|
|
||
| /** @internal */ | ||
| export interface SymbolLinks { | ||
| _symbolLinksBrand: any; |
There was a problem hiding this comment.
I added this primarily to catch an unconditional cast to SymbolLinks from Symbol and decided to leave it in to prevent that from happening in the future.
| @@ -1302,8 +1303,9 @@ const intrinsicTypeKinds: ReadonlyMap<string, IntrinsicTypeKind> = new Map(getEn | |||
| Uncapitalize: IntrinsicTypeKind.Uncapitalize | |||
| })); | |||
|
|
|||
| function SymbolLinks(this: SymbolLinks) { | |||
| } | |||
| const SymbolLinks = class implements SymbolLinks { | |||
There was a problem hiding this comment.
Does using a class mean instances of SymbolLinks will share the same hidden class even if different properties are attached in different orders? If not, how bad is the memory impact of initializing all the possible properties to undefined? (I assume awful because there are so many.) Did you test the class implementation against an object literal implementation?
There was a problem hiding this comment.
Also, what’s the reason for using a class expression here?
There was a problem hiding this comment.
A class declaration would conflict with the interface of the same name. It also lets me remove a cast to any at https://github.com/microsoft/TypeScript/pull/51880/files?show-viewed-files=true&file-filters%5B%5D=#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R2573 and https://github.com/microsoft/TypeScript/pull/51880/files?show-viewed-files=true&file-filters%5B%5D=#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R2320
// before, because `SymbolLinks` is a function
new (SymbolLinks as any)()
// after
new SymbolLinks()
There was a problem hiding this comment.
I wonder how many more of those kinds of things we have that could be switched.
(Those links don't seem to work, but the example is enough to convince me, personally.)
There was a problem hiding this comment.
They don't work because checker isn't expanded by default, you have to expand it manually.
There was a problem hiding this comment.
I didn’t realize we were using a constructor function before. The crux of my question is really, is there a reason why it’s not function createSymbolLinks(): SymbolLinks { return {}; }, and do we know what the difference in performance characteristics between the two is.
There was a problem hiding this comment.
Does using a class mean instances of SymbolLinks will share the same hidden class even if different properties are attached in different orders?
No, whether you are using a class or function has no bearing at the moment because the actual emit from esbuild is a class with no fields. Native classes with native fields would ensure all properties are defined ahead of time, which would help but we can't leverage that currently.
If not, how bad is the memory impact of initializing all the possible properties to
undefined? (I assume awful because there are so many.)
You're essentially adding the space for a heap pointer (roughly 8 bytes) for each property, per object. The best way to reduce that cost is to cut back on what we actually need to store on the object, either through the introduction of bitflags to condense booleans, putting lesser used properties in their own objects, or not storing anything that can be trivially recomputed.
Did you test the class implementation against an object literal implementation?
Not for this specifically, but in general I have yes. There's no real difference in storage, but object literals are far harder to isolate when looking at memory snapshots or v8 traces since they all have Object as their constructor.
There was a problem hiding this comment.
(roughly 8 bytes) for each property
This isn't entirely accurate. IIRC, every object in V8 has an initial structure that is fixed in size and can hold a small number of properties directly. Additional properties beyond that limit are stored elsewhere on the heap. This means every object allocation has a minimum size that accounts for some of the properties.
There was a problem hiding this comment.
I didn’t realize we were using a constructor function before. The crux of my question is really, is there a reason why it’s not
function createSymbolLinks(): SymbolLinks { return {}; }, and do we know what the difference in performance characteristics between the two is.
All object literals share the same constructor so they show up as Object in the debugger, in memory snapshots, and in the v8 trace logs, which make them impossible to differentiate from one another. We should really try to use object literals only for throw-away things like options bags for functions, or destructuring results. Using a class for objects with a longer lifetime is preferred, even if we're just using it as a tag, i.e.:
function createFlowNode() {
return {} as FlowNode; // bad for debugging/snapshots/tracing
}
// vs
function createFlowNode() {
return new FlowNode() as FlowNode; // better for debugging/snapshots/tracing,
// but pretty much the same as {} otherwise
}
// minimum needed to tag the object for diagnostic tooling, though it would be better
// if the object has a stable set of properties we can pre-allocate:
const FlowNode = class FlowNode {};The only downside to this approach is that you end up with a unique starting "map" for each tag (since it points to a different constructor/prototype)
There was a problem hiding this comment.
For example, there are 215 different V8 maps for Object here that I have no insight into without drilling into each one, vs the other constructors in this list:
rbuckton
force-pushed
the
monomorphic-symbol
branch
from
6a5956f to
d7cbf19
Compare
|
Partially related, but the force pushing is making it a little hard to view the changes as they come in; since we squash on merge, is the force pushing required for all of the updates? |
Maybe not, but I generally try to keep related changes isolated to a single commit, and rebase against |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at d7cbf19. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at d7cbf19. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at d7cbf19. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at d7cbf19. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at d7cbf19. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at d7cbf19. You can monitor the build here. Update: The results are in! |
jakebailey
approved these changes
Dec 14, 2022
Comment on lines
-349
to
+350
| HasDecorators, | ||
| hasDecorators, | ||
| HasDecorators, |
There was a problem hiding this comment.
Is some tool doing this? If this is changed, is it likely for this to get flipped back again?
(I am hoping this is moot as of tomorrow-ish, when the next VS Code TS nightly is available with the fix in, and or when we can restore the ordering to be sensitive on main.)
There was a problem hiding this comment.
I did "Sort lines ascending" because auto-imports stuck a few things at the bottom, and organize imports wouldn't fix it.
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
andrewbranch
approved these changes
Dec 14, 2022
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsfelixrieseberg/windows95
|
|
Those ones are all broken on main too, so nothing new there. |
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.
In the same vein as #51682, this makes
Symbol/TransientSymbol/MappedSymbol/ReverseMappedSymbolstable so that all symbol property access becomes monomorphic.The main change is that rather than having all of the
SymbolLinksproperties directly onTransientSymbol, instead they are attached to alinksinternal property that is unconditionally set on everySymbolto ensure the map remains stable.