Reuse Printers with common options#52382
Merged
jakebailey merged 17 commits intomicrosoft:mainfrom Jan 25, 2023
Merged
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @jakebailey, I've started to run the perf test suite on this PR at cd3afe4. You can monitor the build here. Update: The results are in! |
jakebailey
commented
Jan 24, 2023
jakebailey
commented
Jan 24, 2023
jakebailey
changed the title
jakebailey
commented
Jan 24, 2023
Member
Author
|
@typescript-bot perf test this |
Collaborator
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 6d99c8f. You can monitor the build here. Update: The results are in! |
Collaborator
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
Yay! That looks like a few percent improvement ranging from 1-5% ish. |
jakebailey
changed the title
Member
Author
|
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. |
Member
|
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. |
Member
Author
I don't know that it does; quick info and completions appear to use |
Member
Author
|
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. |
jakebailey
commented
Jan 24, 2023
Comment on lines
-733
to
-736
| if (!printer) { | ||
| printer = createPrinter({ removeComments: true }); | ||
| } | ||
| return printer; |
Member
Author
There was a problem hiding this comment.
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 changes
Jan 24, 2023
Member
Author
|
Unfortunately the difference is that all of those other writers mentioned above also pass in |
Member
Author
|
@typescript-bot pack this |
Collaborator
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 4102e95. You can monitor the build here. |
Collaborator
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Member
Author
|
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
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 28dfb91. You can monitor the build here. Update: The results are in! |
Collaborator
|
@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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
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 |
jakebailey
commented
Jan 25, 2023
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); |
Member
Author
There was a problem hiding this comment.
Believe it or not, this is the fastest cache key of the variants I tried.
Member
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Oh, but, perhaps more relevant - we have a getKeyForCompilerOptions function already. It's used by the documentRegistry 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.
Member
Author
There was a problem hiding this comment.
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 was twice 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.
Member
Author
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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).
Member
There was a problem hiding this comment.
All 2 existing usages of getKeyForCompilerOptions 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 and undefined or true or false or the like - is there a perf benefit to writing value === undefined ? "u," : value ? "1," : "0," over ${value},?
Member
Author
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
function bitwise(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)
Member
Author
There was a problem hiding this comment.
That's a good idea (doesn't even have to be a string key either, so we could skip the ""+)
Member
Author
|
I've reverted this back to its approved state (for the beta); no fancy cache, just a shortlist of hardcoded predefined defaults. |
Member
Author
|
@typescript-bot perf test this faster |
Collaborator
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 708216a. You can monitor the build here. Update: The results are in! |
Collaborator
|
@jakebailey Here they are:Comparison Report - main..52382
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Member
Author
|
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 changes
Jan 25, 2023
This was referenced Jan 26, 2023
Merged
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.