-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switched 'ts.performance' to a mixed mode only uses native performance APIs when necessary #42586
Conversation
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.
The original repro had emit time tripled from a TS 4.0 baseline. With this change, they're back within 10% of TS 4.0. Since the repro is noisy and the number of other changes is huge, I'm satisfied.
@typescript-bot perf test this |
Heya @amcasey, I've started to run the perf test suite on this PR at a20bfa2. You can monitor the build here. Update: The results are in! |
@amcasey Here they are:Comparison Report - master..42586
System
Hosts
Scenarios
|
Perf numbers look good to me |
It looks like nothing in the perf suite does a meaningful amount of emit, which is probably why we missed the regression. Should we add something? (Unfortunately, I don't have an example in mind.) |
@amcasey recently discovered that the Web Performance API in NodeJS adds quite a bit more overhead to the total compile time than what we were doing before. This changes our use of native performance APIs in the following ways:
mark
times andmeasure
durations ints.performance
rather than usingPerformanceObserver
.mark
andmeasure
when any of the following are true:--cpu-prof
option specified (to capture user timing events in the profile)--prof
option specified (to capture user timing events in the profile)--generateCpuProfile
option specified (to capture user timing events in the profile)sys.debugMode
is true (since a debugger can start a cpu profile at any time)If none of those cases are true, we will not call the native
mark
andmeasure
methods. I've filed nodejs/diagnostics#464 to track the performance issue we encountered in NodeJS, and it sounds like they plan to have a patch available in the near future that improves their implementation.