perf: reduce GC pressure by hoisting script target features object #55484
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
@typescript-bot perf test this |
|
Heya @DanielRosenwasser, I've started to run the regular perf test suite on this PR at f849917. You can monitor the build here. Update: The results are in! |
|
What are you testing where you're seeing this be costly? Is this likely to be shown in |
I'm trying to make our application build faster. Its webpack5 vue2 typescript app |
|
For a single compilation it's surprising that we even end up doing this more than once. This is only supposed to occur as part of error recovery for an unresolved type. Do you have a lot of unchecked JavaScript code or something? |
…atures object and fixed naming conventions
Here is the call stack:
|
|
@DanielRosenwasser Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I'm certainly confused as to why this is appearing in your call stack; it's trying to produce a nice message for IArguments not existing which could only happen because your code is not typechecking. IArguments is in |
|
I'm sure you've already seen, but you'll want to |
|
It seems to have something to do with |
|
Is it checking with the wrong tsconfig/compilerOptions or something? Missing |
|
You know, I can see how this may be a thing; this is coming out of |
|
Yeah, Of course, this makes me wonder if we could do a lot better about not requesting / doing work for diagnostics during @andrewbranch for interest 😄 In any case, this seems like a totally good optimization regardless; I can see this being checked repeatedly in the editor, for example. |
|
We could probably get a net perf win in |
src/compiler/utilities.ts
Outdated
| @@ -1281,8 +1282,8 @@ export function getInternalEmitFlags(node: Node): InternalEmitFlags { | |||
| export type ScriptTargetFeatures = ReadonlyMap<string, ReadonlyMap<string, string[]>>; | |||
|
|
|||
| /** @internal */ | |||
| export function getScriptTargetFeatures(): ScriptTargetFeatures { | |||
| return new Map(Object.entries({ | |||
| export const getScriptTargetFeatures: () => ScriptTargetFeatures = /* @__PURE__ */ memoize(() => | |||
There was a problem hiding this comment.
I don't think we use this annotation anywhere else - any reason you felt we needed it?
There was a problem hiding this comment.
I had suggested sticking it on the closure inside of the memoize call but they put it here instead (which is roughly equivalent, I guess). Mainly it's that this type is an aliased ReadonlyMap. Not sure why it's named, per se
There was a problem hiding this comment.
I just tacked a small change onto the PR to move the annotation to the other place; wanting to merge this!
…icrosoft#55484) Co-authored-by: Jake Bailey <[email protected]>
This PR reduces GC pressure by hoisting ScriptTargetFeatures instead of creating that object over and over again.