| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
TypeScript Bot (@typescript-bot) perf test |
Sorry, something went wrong.
|
Heya Ron Buckton (@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! |
Sorry, something went wrong.
|
Ron Buckton (@rbuckton)
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) perf test |
Sorry, something went wrong.
|
Heya Ron Buckton (@rbuckton), I've started to run the perf test suite on this PR at 45ebecf. You can monitor the build here. |
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) perf test |
Sorry, something went wrong.
|
Heya Ron Buckton (@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! |
Sorry, something went wrong.
|
Ron Buckton (@rbuckton)
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
Daniel Rosenwasser (@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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| * 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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| // 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| /** @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?
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| @@ -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?
Sorry, something went wrong.
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.)
Sorry, something went wrong.
There was a problem hiding this comment.
Oops, removed the reference, forgot to remove the type.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good. Only question I have is where to put the deprecated originalKeywordKind and isInJSDocNamespace properties.
Sorry, something went wrong.
They're defined in deprecatedCompat, though I can probably move the field definitions there as well. |
Sorry, something went wrong.
In your PR you dropped isIdentifierANonContextualKeyword from compiler/utilities.ts, which I haven't yet done here. I'll have to look more closely to determine if there's anything else. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
| @@ -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.)
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) perf test this |
Sorry, something went wrong.
|
Heya Jake Bailey (@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! |
Sorry, something went wrong.
|
Jake Bailey (@jakebailey)
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
I think moving autoGenerate off of Identifier might be behind the few emit-time increases, given that it is used more frequently than anything else I removed. I may try one more perf run with autoGenerate back on Identifier. |
Sorry, something went wrong.
|
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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) perf test |
Sorry, something went wrong.
|
Heya Ron Buckton (@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! |
Sorry, something went wrong.
|
Ron Buckton (@rbuckton)
CompilerComparison Report - main..52170
System
Hosts
Scenarios
TSServerComparison Report - main..52170
System
Hosts
Scenarios
StartupComparison Report - main..52170
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
I guess reverting autoGenerate doesn't have as much of an impact as I thought, so I'll revert the last commit. |
Sorry, something went wrong.
|
Comparison Report - main..HEAD
System
Hosts
Scenarios
For hopefully more fixed tsserver benchmarks... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
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
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This is a WIP investigation into memory utilization after moving a number of properties off of Identifier.
This has some overlap with #51497