- Notifications
You must be signed in to change notification settings - Fork12.9k
Remove some properties fromIdentifier
#52170
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@typescript-bot perf test |
typescript-bot commentedJan 9, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@rbuckton, I've started to run the perf test suite on this PR atd60b06d. You can monitor the buildhere. 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 commentedJan 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@typescript-bot perf test |
typescript-bot commentedJan 11, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@rbuckton, I've started to run the perf test suite on this PR at45ebecf. You can monitor the buildhere. 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. |
rbuckton commentedJan 11, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm not entirely sure why some of the tsserver benchmarks are so wildly different. I'll need to look into why thereferences test for xstate and thecompletioninfo 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would prefer we call thisidentifierToKeywordKind
or something like that.
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.
Sure. I just named it that way to matchidText
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I may just use a more general name likeidentifierToToken
, 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. |
Uh oh!
There was an error while loading.Please reload this page.
//The entity is part of a JSDoc-style generic. We will usethegap between `typeName` and | ||
// `typeArguments` to report it as a grammar error in the checker. |
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.
Why can't we just report it here?
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.
If we report it here, it goes intoparseDiagnostics
, 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.
src/compiler/types.ts Outdated
/** @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should these be moved todeprecatedCompat
and not used within our codebase?
src/compiler/utilities.ts Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should this be here in this project versus deprecatedCompat?
src/compiler/types.ts Outdated
@@ -1008,7 +1016,6 @@ export type ForEachChildNodes = | |||
/** @internal */ | |||
export type VisitEachChildNodes = |
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.
could we just replace this type withHasChildren
now?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops, removed the reference, forgot to remove the type.
Uh oh!
There was an error while loading.Please reload this page.
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.
Looks good. Only question I have is where to put the deprecated originalKeywordKind and isInJSDocNamespace properties.
They're defined in |
In your PR you dropped |
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.
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.
Uh oh!
There was an error while loading.Please reload this page.
src/compiler/types.ts Outdated
@@ -1008,7 +1016,6 @@ export type ForEachChildNodes = | |||
/** @internal */ | |||
export type VisitEachChildNodes = |
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.
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.)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
@typescript-bot perf test this |
typescript-bot commentedJan 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@jakebailey, I've started to run the perf test suite on this PR at85e5453. You can monitor the buildhere. 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 |
typescript-bot commentedJan 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Heya@rbuckton, I've started to run the perf test suite on this PR at107d090. You can monitor the buildhere. 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 |
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
|
This is a WIP investigation into memory utilization after moving a number of properties off of
Identifier
.This has some overlap with#51497