- Notifications
You must be signed in to change notification settings - Fork12.9k
Reuse Printers with common options#52382
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
Merged
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or 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 perf test this |
Collaborator
typescript-bot commentedJan 24, 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 atcd3afe4. You can monitor the buildhere. Update:The results are in! |
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 |
Collaborator
typescript-bot commentedJan 24, 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 at6d99c8f. You can monitor the buildhere. Update:The results are in! |
@jakebailey Here they are:CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@jakebailey Here they are:CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Yay! That looks like a few percent improvement ranging from 1-5% ish. |
I've rechecked the numbers from above; I can reproduce the 2% overall benefit on TFS: Which at least seems to indicate that the rest of the numbers are real. But, I see no improvement when compiling our codebase, which to me indicates that this is probably just improving the performance of error messages. I think this comes down to the fact that our benchmarks are so old that none of them compile anymore. That isn't to say that improving that case isn't worthwhile, though. |
I mentioned this over chat - I am curious whether things like long completion lists of auto-imports benefit here. I don't know if our perf suite captures that sort of thing though. |
I don't know that it does; quick info and completions appear to use |
After this PR, the only places we arbitrarily come up with printers is:
I think these are all places that should reuse printers, but they are tricky because they pass a lot more options in; all of the ones I've touched in this PR only pass 0-2 of them so they're easy to enumerate. |
Comment on lines -733 to -736
| if(!printer){ | ||
| printer=createPrinter({removeComments:true}); | ||
| } | ||
| returnprinter; |
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.
Here's the code which already reused the printer; that being said, it only cached it once for each symbol, so it could totally be that this is a big improvement. Will have to rerun the benchmarks.
weswigham approved these changesJan 24, 2023
Unfortunately the difference is that all of those other writers mentioned above also pass in |
@typescript-bot pack this |
Collaborator
typescript-bot commentedJan 25, 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 tarball bundle task on this PR at4102e95. You can monitor the buildhere. |
Collaborator
typescript-bot commentedJan 25, 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.
Hey@jakebailey, I've packed this intoan installable tgz. You can install it for testing by referencing it in your and then running There is also a playgroundfor this build and annpm module you can use via |
I'm not sure I'm going to keep this latest commit, but I want to try and see if it's worse in some way. @typescript-bot perf test this |
Collaborator
typescript-bot commentedJan 25, 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 at28dfb91. You can monitor the buildhere. Update:The results are in! |
@jakebailey Here they are:CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Performance of this more scalable / fancier version appear to be the same; that's good and makes some sense given this is the same sort of thing we do for other caches in the checker. I have a more invasive change which changes printer to allow you to pass in |
src/compiler/emitter.ts Outdated
Comment on lines 1354 to 1359
| const key = keyBool(o.removeComments) | ||
| + keyBool(o.neverAsciiEscape) | ||
| + keyBool(o.omitTrailingSemicolon) | ||
| + keyNum(o.module) | ||
| + keyNum(o.target) | ||
| + keyNum(o.newLine); |
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.
Believe it or not, this is the fastest cache key of the variants I tried.
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.
It's actually faster than using a simple native template string like
`${o.removeComments}|${o.neverAsciiEscape}|${o.omitTrailingSemicolon}|${o.module}|${o.target}|${o.newLine}`? Feels odd.
Member
weswighamJan 25, 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.
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.
Oh, but, perhaps more relevant - we have agetKeyForCompilerOptions function already. It's used by thedocumentRegistry in the language service and in the module resolver. If there is actually a big difference in time spent making the key, maybe some changes to it would be warranted as well? I'd certainly prefer it if we reused the existing option-key-generating-mechanism if we could.
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.
So, two things:
- Writing out
keyBool(...) + "|" + keyBool(...) + "|" + ...was just as fast as writing out${keyBool(...)}|${keyBool(...)}.... - Moving the separators into the "keys" (i.e. how I did
u,1,0,) instead of putting them the template wastwice as fast.
So, I opted for the variant which doesn't make the line go way way to the right as the number of parts increases.
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.
That being said it's still like millions of iterations per second so unlikely to matter; I was really sad to find that[..., ..., ...].join("|") was an order of magnitude slower than these methods.
I'll go take a look at that other function.
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.
Ah,getKeyForCompilerOptions is quite literally what I said was slow above; given the arbitrary nature of the big list of compiler keys, I'm not sure I could change that.
The checker itself does have a few internal caches which do the hardcoded thing you said (and I did first), hard to say if they'd really benefit.
I'm happy to make this whatever makes sense, I just knew for this one I had to combine like 6 things and the length was getting unwieldy (so I resorted to helpers and multiple lines).
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.
All 2 existing usages ofgetKeyForCompilerOptions actually use a fixed list of keys - longer lists, to be sure, made by filtering the full list of options by a boolean flag set on the option, but still fixed lists. In module resolution, we actually use these keys quite a lot - if it's bad enough to matter here, I'd think it'd matter there, too.
Writing out keyBool(...) + "|" + keyBool(...) + "|" + ... was just as fast as writing out ${keyBool(...)}|${keyBool(...)}....
As for the simpler case, my focus on moreso why the coercion functions were needed at all, and a simple string concatenation wouldn't suffice. Since the order is fixed, it's not like you need to distringuish between strings andundefined ortrue orfalse or the like - is there a perf benefit to writingvalue === undefined ? "u," : value ? "1," : "0," over${value},?
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.
Ah, I see. I tested that one out and it was middling in performance.
https://jsbench.me/prldbzl6dj/2 is what I was using to test, but I also can just revert these last few commits and go back to the version which hardcodes the 4 most common ones.
Member
weswighamJan 25, 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.
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.
For your future cache key micro-optimizing endeavors: If string concatenation is the slow bit, seems like it's best to skip on strings almost entirely:
functionbitwise(o){return""+((o.removeComments ?1 :0)|(o.neverAsciiEscape ?2 :0)|(o.omitTrailingSemicolon ?4 :0)|(o.module<<4)// 0-199|(o.target<<12)// 0-100|(o.newLine<<20));// 0-1}
is ~5% faster than the string-based next best 😛 (assuming it's OK to treat undefined and false the same, which it might be, might not be in this case)
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.
That's a good idea (doesn't even have to be a string key either, so we could skip the ""+)
I've reverted this back to its approved state (for the beta); no fancy cache, just a shortlist of hardcoded predefined defaults. |
@typescript-bot perf test this faster |
Collaborator
typescript-bot commentedJan 25, 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 abridged perf test suite on this PR at708216a. You can monitor the buildhere. Update:The results are in! |
@jakebailey Here they are:Comparison Report - main..52382
System
Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Perf seems the same as the first run (though I only checked node 16 via the short test; node 18 appears to do better). Are we good with this PR in this state for the beta? |
DanielRosenwasser approved these changesJan 25, 2023
This was referencedJan 26, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
4 participants
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#52350 I noticed that
declarationEmitPrivatePromiseLikeInterfacewas running quite slowly.I ran this single test through pprof and found that 20% of the time is spent just doing
createPrinter.With this change, the test goes from ~6s to ~4s on my machine, and now that call is gone:
It seems to me like we have a lot of printers created with static options, although I'm guessing a lot aren't in a hot path like this one.