Remove some properties from Identifier
#52170
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
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at d60b06d. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 45ebecf. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@DanielRosenwasser, this is missing a few changes from #51497. Let me know if you want me to merge those changes in or if what I put together is acceptable. |
|
I'm not entirely sure why some of the tsserver benchmarks are so wildly different. I'll need to look into why the references test for xstate and the completioninfo test for Compiler-Unions had such a regression, given that this change improves performance in a number of other tsserver tests. We are measuring in ms in these cases. Given the scale I'm not sure how sensitive these tests are to minor changes. |
src/compiler/utilitiesPublic.ts
Outdated
| * If the text of an Identifier matches a keyword (including contextual and TypeScript-specific keywords), returns the | ||
| * SyntaxKind for the matching keyword. | ||
| */ | ||
| export function idKeyword(node: Identifier) { |
There was a problem hiding this comment.
I would prefer we call this identifierToKeywordKind or something like that.
There was a problem hiding this comment.
Sure. I just named it that way to match idText so that they would be sorted together in completions. The FP-like design of the compiler generally lends itself to using common prefixes to group related functionality together.
There was a problem hiding this comment.
I may just use a more general name like identifierToToken, since we don't necessarily validate identifier text, nor do we check that the result is limited to only keyword tokens.
|
Could you summarize on which changes from #51497 are missing here? From a glance it seems like you have them all and more, but I'm not sure. |
Comment on lines
+3457
to
+3458
| // The entity is part of a JSDoc-style generic. We will use the gap between `typeName` and | ||
| // `typeArguments` to report it as a grammar error in the checker. |
There was a problem hiding this comment.
Why can't we just report it here?
There was a problem hiding this comment.
If we report it here, it goes into parseDiagnostics, and that turns off a bunch of other checking in checker.ts. In general I'd prefer if we moved a lot of the grammar checks to parser.ts so that we can free up space on a number of nodes, but that might impact incremental parse. I've opted to push off any grammar-check related changes to a separate investigation.
jakebailey
reviewed
Jan 11, 2023
src/compiler/types.ts
Outdated
Comment on lines
1688
to
1691
| /** @deprecated Use `idKeyword(identifier)` instead. */ | ||
| readonly originalKeywordKind?: SyntaxKind; // Original syntaxKind which get set so that we can report an error later | ||
| /** @deprecated Use `identifier.flags & NodeFlags.IdentifierIsInJSDocNamespace` instead. */ | ||
| readonly isInJSDocNamespace?: boolean; // if the node is a member in a JSDoc namespace. |
There was a problem hiding this comment.
Should these be moved to deprecatedCompat and not used within our codebase?
src/compiler/utilities.ts
Outdated
Comment on lines
7338
to
7350
| Object.defineProperties(Identifier.prototype, { | ||
| originalKeywordKind: { | ||
| get(this: Identifier) { | ||
| return stringToToken(this.escapedText as string); | ||
| } | ||
| }, | ||
| isInJSDocNamespace: { | ||
| get(this: Identifier) { | ||
| // NOTE: Returns `true` or `undefined` to match previous possible values. | ||
| return this.flags & NodeFlags.IdentifierIsInJSDocNamespace ? true : undefined; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Should this be here in this project versus deprecatedCompat?
sandersn
reviewed
Jan 11, 2023
src/compiler/types.ts
Outdated
| @@ -1008,7 +1016,6 @@ export type ForEachChildNodes = | |||
| /** @internal */ | |||
| export type VisitEachChildNodes = | |||
There was a problem hiding this comment.
could we just replace this type with HasChildren now?
There was a problem hiding this comment.
This was resolved without comment; do we still need this type? (I don't care either way, I was just interested in the rationale to leave it.)
There was a problem hiding this comment.
Oops, removed the reference, forgot to remove the type.
sandersn
approved these changes
Jan 12, 2023
They're defined in |
In your PR you dropped |
jakebailey
approved these changes
Jan 12, 2023
There was a problem hiding this comment.
LGTM with comments.
Do we need to re-perf-test just to make sure that the leftover duplicate prototype modification in the compiler project doesn't matter?
I'm not entirely sure why some of the tsserver benchmarks are so wildly different. I'll need to look into why the references test for xstate and the completioninfo test for Compiler-Unions had such a regression, given that this change improves performance in a number of other tsserver tests.
I'm looking into this one; I think this is new after my tuning changes. The bi-modal-ness of the data to me seems to imply that tsserver is somehow managing to do some work on the wrong processor even though that should be impossible.
src/compiler/types.ts
Outdated
| @@ -1008,7 +1016,6 @@ export type ForEachChildNodes = | |||
| /** @internal */ | |||
| export type VisitEachChildNodes = | |||
There was a problem hiding this comment.
This was resolved without comment; do we still need this type? (I don't care either way, I was just interested in the rationale to leave it.)
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 85e5453. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think moving |
|
Something's definitely wrong with the server benchmarks at the moment; watching them run, some CPU time appears to leave the intended CPU core. Here's the server results on my perf machine without any of the tuning options: Comparison Report - main..HEAD
System
Hosts
Scenarios
But without tuning, it's hard to say how much of this is real. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 107d090. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I guess reverting |
8 tasks
|
Comparison Report - main..HEAD
System
Hosts
Scenarios
For hopefully more fixed tsserver benchmarks... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I've run it again on this machine in a different order and got the same result, effectively. Comparison Report - main..HEAD
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
This is a WIP investigation into memory utilization after moving a number of properties off of
Identifier.This has some overlap with #51497