Migrate 'ts.performance' to use native performance hooks when available #40593
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
rbuckton
force-pushed
the
nativePerformanceHooks
branch
from
2929b47 to
4b5662b
Compare
src/compiler/performance.ts
Outdated
| enabled = true; | ||
| profilerStart = timestamp(); | ||
| if (!enabled) { | ||
| perfHooks ||= tryGetNativePerformanceHooks() || ShimPerformance?.createPerformanceHooksShim(timestamp); |
There was a problem hiding this comment.
Under what circumstances is the shim un/available?
There was a problem hiding this comment.
The shim is only loaded in the language service/tsserver. tsc.ts never loads the shim. That's the same behavior we use for the Map and Set shims.
There was a problem hiding this comment.
Interesting. Does the LS use the perf code?
There was a problem hiding this comment.
Not currently, but in theory it could. Especially if we end up with a way to have these user timings appear in a trace, and wanted to be able to initiate a trace against the language service from within the editor. This is actually a fairly reasonable possibility, since VS Code does have this functionality (although, IIRC, it only currently profiles the "in-process" extension, not the language service):
(from the Developer: Show Running Extensions command).
There was a problem hiding this comment.
Are the shims for Monaco or something? I'm pretty sure both VS and VS Code are on node >= 10?
There was a problem hiding this comment.
The shim would be for any non-node host, such as Monaco in the browser, or if anyone else runs TS in the browser. The ts.performance namespace is marked /* @internal */ so its unlikely anyone is actually using it in those scenarios, however.
It doesn't look like you can trigger it on the playground with // @diagnostics: true (as far as I can tell, maybe @orta knows for sure). I'm happy to drop the shim if that's what everyone else wants.
There was a problem hiding this comment.
That will definitely set the compiler option in the TSServer in the playground:
From the looks of caniuse.com for this API, the last browser to add the API was Safari in 2017 - which might fall into Monaco's support window. They basically say "any desktop browser but IE".
It's outside the TS website's support window though (I use this API at the bottom of handbook pages but skip showing it on "ancient" browsers)
So, I don't really have a definitive answer I'm afraid
|
Assuming it's not impossible, what's the experience if you pass |
|
With this change? |
|
@rbuckton What if we made it |
|
I can change the error message. It will only appear if You use |
|
The no-op's a bit nicer because you'll still get file counts and things (I think?), but I'm not that concerned about polishing that scenario. Whatever's easiest. |
|
Is this something that we want to take between 4.1-beta and RC, or is this something that should wait for 4.2? Its not a "breaking change" per se, as the only error is one that would be reported in an unsupported environment (i.e. running |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at dfcb084. You can monitor the build here. Update: The results are in! |
|
@rbuckton My vote would be 4.2, since I don't think it lights up anything critical. |
|
4.2 is fine with me, I can leave this in a Draft state until after the RC goes out. |
|
I'm removing the |
|
I'll leave this here for vote-by-reaction:
|
|
@rbuckton Here they are:Comparison Report - master..40593
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
If not merging this now, would it make sense to merge the smaller change I made (#40590) to use |
| @@ -43,7 +43,7 @@ namespace ts.tscWatch { | |||
| return createWatchProgram(compilerHost); | |||
| } | |||
|
|
|||
| const elapsedRegex = /^Elapsed:: [0-9]+ms/; | |||
| const elapsedRegex = /^Elapsed:: [0-9]+(?:\.\d+)?ms/; | |||
There was a problem hiding this comment.
(Kind of weird to have [0-9] on one side, and \d on the other... Also in virtualFileSystemWithWatch.ts)
| /** Gets a timestamp with (at least) ms resolution */ | ||
| export const timestamp = | ||
| nativePerformance ? () => nativePerformance.now() : | ||
| Date.now ? Date.now : |
There was a problem hiding this comment.
I think that I copied this in my earlier comment, so just in case: didn't you say that Date.now should be wrapped for proper invocation in browsers?
elibarzilay
force-pushed
the
nativePerformanceHooks
branch
from
b1323fe to
68806c6
Compare
|
(Rebased on |
|
While I had previously suggested that this should probably wait until 4.2, recent discussions with @RyanCavanaugh made me think that this was fine for 4.1 bar, especially if it was something we wanted to build on for another 4.1 change. |
rbuckton
force-pushed
the
nativePerformanceHooks
branch
from
4f94f87 to
064cb52
Compare
rbuckton
force-pushed
the
nativePerformanceHooks
branch
from
064cb52 to
c5800d1
Compare
|
@amcasey can you take one more pass over this before I merge? |
|
Will do. |
The Web Performance User Timings API has been available in NodeJS since 8.4, and is supported by pretty much every major browser (except for Opera Mini).
This PR refactors
ts.performanceto leverage either the browser version or the version from Node'sperf_hooksmodule when available. There's also a shim that's only loaded in the language service, but I may end up removing the shim and just have thets.performanceAPI be a noop if there's no native support available.