-
Notifications
You must be signed in to change notification settings - Fork 78
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
perf_hooks.PerformanceObserver is very slow #464
Comments
Also note, when running TypeScript without
Another note is that the difference between using and not using |
Yep, the implementation could definitely use a performance overhaul. I've had it on my list for a while but haven't been able to prioritize it. There are a couple of key challenges on it, the most significant of which is just the cost of creating the |
We use marks for two purposes:
We use measures to calculate the total amount of time we spend in various operations, in aggregate. For example, There were a few things we've noticed when running microbenchmarks for
@amcasey may be able to provide additional details, as he's been the one investigating the performance impact. |
Fantastic. That makes sense. I've already got some ideas on improving things. |
Ok, so nodejs/node#37136 has a complete rework of the user timing implementation that improves performance significantly. It is, however, semver-major. Assuming we're able to get it landed in the main repo I'll see if there's a way to backport a subset of the changes in a semver-minor PR. |
* Update the user timing implementation to conform to User Timing Level 3. * Reimplement user timing and timerify with pure JavaScript implementations * Simplify the C++ implementation for gc and http2 perf * Runtime deprecate additional perf entry properties in favor of the standard detail argument * Disable the `buffered` option on PerformanceObserver, all entries are queued and dispatched on setImmediate. Only entries with active observers are buffered. * This does remove the user timing and timerify trace events. Because the trace_events are still considered experimental, those can be removed without a deprecation cycle. They are removed to improve performance and reduce complexity. Old: `perf_hooks/usertiming.js n=100000: 92,378.01249733355` New: perf_hooks/usertiming.js n=100000: 270,393.5280638482` PR-URL: #37136 Refs: nodejs/diagnostics#464 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made. |
I neglected to look into the solution in nodejs/node#37136. Unfortunately, it depends on |
Yeah I've been thinking about options there. I prefer deferring the observer by default but I'm thinking about adding an option to force it sync or have it use the microtaskqueue instead. |
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after #37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: #39297 Reviewed-By: James M Snell <jasnell@gmail.com>
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after #37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: #39297 Reviewed-By: James M Snell <jasnell@gmail.com>
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after #37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: #39297 Reviewed-By: James M Snell <jasnell@gmail.com>
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after #37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: #39297 Reviewed-By: James M Snell <jasnell@gmail.com>
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after nodejs#37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: nodejs#39297 Reviewed-By: James M Snell <jasnell@gmail.com>
All API introduced in this PR are compliant with web [performance-timeline](https://w3c.github.io/performance-timeline) spec. "performance-timeline" is listed as supported web spec in the doc https://nodejs.org/docs/latest/api/perf_hooks.html#perf_hooks_performance_measurement_apis. Changes summary: 1. Add new supported wpt test subsets: user-timing and performance-timeline. 2. Add support for `Performance.getEntries`, `Performance.getEntriesByName` and `Performance.getEntriesByType` to synchronously fetch buffered performance entries. This means the user should invoke `Performance.clearMarks` and `Performance.clearMeasures` to clear buffered entries to prevent from those entries been kept alive forever. 3. Add support (again after #37136) for `buffered` flags for `PerformanceObserver`. 3. Fixes `PerformanceMark` and `PerformanceMeasure` wpt compliance issues. 4. Only user-created performance entries will be buffered globally. This behavior should be compliant with https://w3c.github.io/timing-entrytypes-registry/#registry. With the new ability to fetch user-created performance entries synchronously, the issues raised in nodejs/diagnostics#464 (comment) could also be fixed. PR-URL: #39297 Reviewed-By: James M Snell <jasnell@gmail.com>
Back in October of 2020, TypeScript switched (1, 2) from using our own performance measurement API to using
perf_hooks.performance
andperf_hooks.PerformanceObserver
. Since then we have had some users reporting that building with TypeScript using our--diagnostics
or--extendedDiagnostics
flags (which enables on our performance measurement functionality) has caused build time to regress by an additional 20% in some cases prior to this change.We had two goals with our original change:
--cpu-prof
with the hopes that we could generate cpu profiles with user timings information.However, we may be forced to revert to our previous custom implementation due to the significant overhead incurred using
PerformanceObserver
.Our expectation would be that using performance measurement APIs shouldn't significantly impact the performance of observed code (though we are aware that its impossible for performance measurement to be completely free).
On a side note, I spoke with @devsnek offline and they suggested we try our scenario using a recent NodeJS 15 build and the
--turbo-fast-api-calls
flag. The table below reflects the results of running the compiler with and without this flag on different NodeJS versions when compilingant-design
/antd
:--turbo-fast-api-calls
was passed to NodeJS--extendedDiagnostics
perf_hooks.performance
andperf_hooks.PerformanceObserver
I'd like to point out several take-aways from the table above:
perf_hooks.PerformanceObserver
is generally about 17.5% slower in the above benchmarks than our prior custom measurement API.--turbo-fast-api-calls
actually makes things worse rather than better.In the event I'm doing something wrong with our use of
PerformanceObserver
, you can find the source for our performance wrapper (which essentially just forwards calls tomark
andmeasure
and uses aPerformanceObserver
to capture information) here: https://github.com/microsoft/TypeScript/blob/cdd11e96ad3dd039da76c1506e35bc7e74dd57f1/src/compiler/performance.tsThe text was updated successfully, but these errors were encountered: