{{ message }}
Reuse Printers with common options - #52382
Merged
Jake Bailey (jakebailey) merged 17 commits intoJan 25, 2023
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
TypeScript Bot (@typescript-bot) perf test this |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Heya Jake Bailey (@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! |
Sorry, something went wrong.
Comment thread
src/compiler/checker.ts
Outdated
Comment thread
src/compiler/checker.ts
Outdated
Comment thread
src/compiler/emitter.ts
Copy link
Copy Markdown
Member
Author
|
TypeScript Bot (@typescript-bot) perf test this |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Heya Jake Bailey (@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! |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Jake Bailey (@jakebailey)
CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Jake Bailey (@jakebailey)
CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
Yay! That looks like a few percent improvement ranging from 1-5% ish. |
Sorry, something went wrong.
Jake Bailey (jakebailey)
marked this pull request as ready for review
January 24, 2023 06:09
Copy link
Copy Markdown
Member
Author
|
I've rechecked the numbers from above; I can reproduce the 2% overall benefit on TFS: Benchmark 1: main
Time (mean ± σ): 17.619 s ± 0.045 s [User: 17.314 s, System: 0.279 s]
Range (min … max): 17.511 s … 17.702 s 20 runs
Warning: Ignoring non-zero exit code.
Benchmark 2: 52382
Time (mean ± σ): 17.301 s ± 0.042 s [User: 17.016 s, System: 0.259 s]
Range (min … max): 17.225 s … 17.402 s 20 runs
Warning: Ignoring non-zero exit code.
Summary
'52382' ran
1.02 ± 0.00 times faster than 'main'
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. |
Sorry, something went wrong.
Copy link
Copy Markdown
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. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
I don't know that it does; quick info and completions appear to use getSymbolDisplayPartsDocumentationAndSymbolKind, which already uses an optimization like this (I'm going to update the PR to consolidate it). |
Sorry, something went wrong.
Copy link
Copy Markdown
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. |
Sorry, something went wrong.
Comment thread
src/services/symbolDisplay.ts
Comment on lines
-733
to
-736
| if (!printer) { | ||
| printer = createPrinter({ removeComments: true }); | ||
| } | ||
| return printer; |
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Wesley Wigham (weswigham)
approved these changes
Jan 24, 2023
Copy link
Copy Markdown
Member
Author
|
Unfortunately the difference is that all of those other writers mentioned above also pass in PrintHandlers which can't be memoized. I'm a little confused though, as that appears to always be writer which is always passed in as an argument instead. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
TypeScript Bot (@typescript-bot) pack this |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at 4102e95. You can monitor the build here. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so: {
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/143917/artifacts?artifactName=tgz&fileId=69B086734D27136ECC49E773E04D82909A0A11EF02E3E36F3631B427C63D71F602&fileName=/typescript-5.0.0-insiders.20230125.tgz"
}
}
and then running npm install. There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.0.0-pr-52382-14".; |
Sorry, something went wrong.
Copy link
Copy Markdown
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 (@typescript-bot) perf test this |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Heya Jake Bailey (@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! |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Jake Bailey (@jakebailey)
CompilerComparison Report - main..52382
System
Hosts
Scenarios
TSServerComparison Report - main..52382
System
Hosts
Scenarios
StartupComparison Report - main..52382
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
Copy link
Copy Markdown
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 handlers in the internal APIs which would fix the rest, but I'll try that on another experimental PR first. |
Sorry, something went wrong.
Comment thread
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); |
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
Believe it or not, this is the fastest cache key of the variants I tried.
Sorry, something went wrong.
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Copy link
Copy Markdown
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).
Sorry, something went wrong.
Copy link
Copy Markdown
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},?
Sorry, something went wrong.
Copy link
Copy Markdown
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.
Sorry, something went wrong.
Copy link
Copy Markdown
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)
Sorry, something went wrong.
Copy link
Copy Markdown
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 ""+)
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
I've reverted this back to its approved state (for the beta); no fancy cache, just a shortlist of hardcoded predefined defaults. |
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
|
TypeScript Bot (@typescript-bot) perf test this faster |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Heya Jake Bailey (@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! |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
|
Jake Bailey (@jakebailey) Comparison Report - main..52382
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
Copy link
Copy Markdown
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? |
Sorry, something went wrong.
Daniel Rosenwasser (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.
Reviewers
Assignees
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 declarationEmitPrivatePromiseLikeInterface was 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.