perf_hooks: add ability to fetch performance entries synchronously #39297

legendecas · 2021-07-07T16:42:57Z

All API introduced in this PR are compliant with web 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:

Add new supported wpt test subsets: user-timing and performance-timeline.
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.
Add support (again after perf_hooks: complete overhaul of the implementation #37136) for buffered flags for PerformanceObserver.
Fixes PerformanceMark and PerformanceMeasure wpt compliance issues
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, I believe issues raised in nodejs/diagnostics#464 (comment) could also be fixed.

Performance comparison:

                                                     confidence improvement accuracy (*)   (**)  (***)
perf_hooks/usertiming.js observe='all' n=100000            ***     -3.69 %       ±1.77% ±2.36% ±3.09%
perf_hooks/usertiming.js observe='measure' n=100000        ***    -23.74 %       ±2.45% ±3.27% ±4.28%

jasnell · 2021-07-07T19:27:51Z

I'm not convinced we should do this. The persistent marks and measures were removed to avoid the Very Likely chance that they would become a memory leak in server code. If the user is not very careful about clearing those, it'll become a problem very quickly.

legendecas · 2021-07-08T00:41:58Z

@jasnell The persistent marks and measures were removed

I'm confused about this statement. The marks are always persistent either with the change of #37136, and users should clear them with Performance.clearMarks in Node.js already. Regarding the spec, the APIs are designed to be used with clearMarks and clearMeasures. So I do not get your point of memory leaks problem. If that is a problem, then that is already a real problem but not introduced in this PR.

lib/internal/perf/observe.js

jasnell · 2021-07-08T21:12:51Z

@legendecas :

I'm confused about this statement. The marks are always persistent either with the change of #37136, and users should clear them with Performance.clearMarks in Node.js already. Regarding the spec, the APIs are designed to be used with clearMarks and clearMeasures. So I do not get your point of memory leaks problem. If that is a problem, then that is already a real problem but not introduced in this PR.

The marks are stored, yes, using as little memory as possible. The associated PerformanceEntry objects are not, and that's what I'm referring to as a potential memory leak here. Currently, the longest time we queue up the PerformanceEntry objects is a single event loop turn while they are collected before they are emitted to the PerformanceObserver. It's a potential footgun if people aren't aware (keep in mind that users may not even know that they need to call clearMarks() if a buggy dependency is filling up the queue.

Perhaps we could add a process.emitWarning() in here if the queue grows past a given threshhold?

legendecas · 2021-07-12T00:18:59Z

@jasnell Perhaps we could add a process.emitWarning() in here if the queue grows past a given threshhold?

This sounds pretty good! I'll do an update.

lib/internal/perf/observe.js

nodejs-github-bot · 2021-07-20T02:38:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/39144/

legendecas · 2021-07-25T15:43:59Z

Landed in 062f8e3

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>

targos · 2021-07-25T15:59:20Z

this seems semver-minor?

legendecas · 2021-07-25T16:15:17Z

this seems semver-minor?

Yeah, it doesn't introduce breaking changes to the v16.x line. But it depends on #37136 and changed various API semantics (compared to v12.x and v14.x) to be compliant with web specs, it should not be landed on v12.x and v14.x.

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>

Notable changes: build: * (SEMVER-MINOR) reset embedder string to "-node.0" (Michaël Zasso) #39470 deps: * (SEMVER-MINOR) restore minimum ICU version to 68 (Michaël Zasso) #39470 * (SEMVER-MINOR) make V8 9.2 abi-compatible with 9.0 (Michaël Zasso) #39470 * (SEMVER-MINOR) update V8 to 9.2.230.21 (Michaël Zasso) #39470 inspector: * mark as stable (Gireesh Punathil) #37748 perf_hooks: * (SEMVER-MINOR) web performance timeline compliance (legendecas) #39297 punycode: * add pending deprecation (Antoine du Hamel) #38444 repl: * (SEMVER-MINOR) enable --experimental-repl-await /w opt-out (hemanth.hm) #34733 PR-URL: TODO

The option buffered is not about queueing the PerformanceEntrys with an event loop task or not. The option buffered in the spec is about filling the observer with the global PerformanceEntry buffer. The current (and the spec) behavior is different with Node.js version <= v16.0.0. PR-URL: #39514 Refs: https://w3c.github.io/performance-timeline/#observe-method Refs: https://nodejs.org/dist/latest-v14.x/docs/api/perf_hooks.html#perf_hooks_performanceobserver_observe_options Refs: #39297 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@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>

This is a security release. Notable Changes: - CVE-2021-22930: Use after free on close http2 on stream canceling (High) [#39423](#39423) - (SEMVER-MINOR) deps: update V8 to 9.2.230.21 (Michaël Zasso) [#39470](#39470) - inspector: mark as stable (Gireesh Punathil) [#37748](#37748) - (SEMVER-MINOR) perf_hooks: web performance timeline compliance (legendecas) [#39297](#39297) - punycode: add pending deprecation (Antoine du Hamel) [#38444](#38444) - (SEMVER-MINOR) repl: enable --experimental-repl-await /w opt-out (hemanth.hm) [#34733](#34733) PR-URL: #39534

BethGriggs · 2021-07-29T16:41:53Z

This seemed to cause a test failure with radium that was surfaced when running CITGM on v16.6.0 - see #39534 (comment).

legendecas · 2021-07-29T16:52:56Z

This seemed to cause a test failure with radium that was surfaced when running CITGM on v16.6.0 - see #39534 (comment).

#39532 is supposed to fix the issue.

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>

The option buffered is not about queueing the PerformanceEntrys with an event loop task or not. The option buffered in the spec is about filling the observer with the global PerformanceEntry buffer. The current (and the spec) behavior is different with Node.js version <= v16.0.0. PR-URL: #39514 Refs: https://w3c.github.io/performance-timeline/#observe-method Refs: https://nodejs.org/dist/latest-v14.x/docs/api/perf_hooks.html#perf_hooks_performanceobserver_observe_options Refs: #39297 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@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>

The option buffered is not about queueing the PerformanceEntrys with an event loop task or not. The option buffered in the spec is about filling the observer with the global PerformanceEntry buffer. The current (and the spec) behavior is different with Node.js version <= v16.0.0. PR-URL: #39514 Refs: https://w3c.github.io/performance-timeline/#observe-method Refs: https://nodejs.org/dist/latest-v14.x/docs/api/perf_hooks.html#perf_hooks_performanceobserver_observe_options Refs: #39297 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

github-actions bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 7, 2021

legendecas requested a review from jasnell July 7, 2021 16:45

legendecas added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Jul 7, 2021

aduh95 reviewed Jul 8, 2021

View reviewed changes

lib/internal/perf/observe.js Outdated Show resolved Hide resolved

lib/internal/perf/observe.js Outdated Show resolved Hide resolved

lib/internal/perf/observe.js Outdated Show resolved Hide resolved

legendecas mentioned this pull request Jul 16, 2021

doc: performanceObserver.observe(options) missing buffered #39314

Closed

1 task

legendecas force-pushed the wpt/user-timing branch from 1195838 to df46dda Compare July 18, 2021 13:48

aduh95 reviewed Jul 18, 2021

View reviewed changes

lib/internal/perf/observe.js Outdated Show resolved Hide resolved

legendecas force-pushed the wpt/user-timing branch from df46dda to 9b0d156 Compare July 18, 2021 13:56

jasnell approved these changes Jul 18, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

perf_hooks: web performance timeline compliance

1f6218e

legendecas force-pushed the wpt/user-timing branch from 188e801 to 1f6218e Compare July 19, 2021 14:52

This comment has been minimized.

Sign in to view

fixup! lib: undefined values in dictionary should be ignored

58ac81a

This comment has been minimized.

Sign in to view

legendecas closed this Jul 25, 2021

legendecas deleted the wpt/user-timing branch July 25, 2021 15:44

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 25, 2021

legendecas added dont-land-on-v12.x labels Jul 25, 2021

aduh95 mentioned this pull request Jul 25, 2021

doc: add PerformanceObserver buffered document #39514

Closed

BethGriggs mentioned this pull request Jul 26, 2021

v16.6.0 proposal #39534

Merged

danielleadams mentioned this pull request Aug 16, 2021

v16.7.0 release proposal #39782

Merged

This was referenced Nov 3, 2021

[7.16.0 BC2] Potential memory leak observed - possibly caused by perf_hooks usage elastic/kibana#116636

Closed

Undocumented breaking change in a minor #40755

Closed

unlikelyzero mentioned this pull request Jul 8, 2022

[Performance] Re-enable Performance Marks nasa/openmct#5466

Open

luketaher mentioned this pull request Sep 4, 2024

performance.getEntriesByName ignores optional type argument #54766

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf_hooks: add ability to fetch performance entries synchronously #39297

perf_hooks: add ability to fetch performance entries synchronously #39297

legendecas commented Jul 7, 2021 •

edited

Loading

jasnell commented Jul 7, 2021

legendecas commented Jul 8, 2021 •

edited

Loading

jasnell commented Jul 8, 2021

legendecas commented Jul 12, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jul 20, 2021

legendecas commented Jul 25, 2021

targos commented Jul 25, 2021

legendecas commented Jul 25, 2021

BethGriggs commented Jul 29, 2021

legendecas commented Jul 29, 2021

perf_hooks: add ability to fetch performance entries synchronously #39297

perf_hooks: add ability to fetch performance entries synchronously #39297

Conversation

legendecas commented Jul 7, 2021 • edited Loading

jasnell commented Jul 7, 2021

legendecas commented Jul 8, 2021 • edited Loading

jasnell commented Jul 8, 2021

legendecas commented Jul 12, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jul 20, 2021

legendecas commented Jul 25, 2021

targos commented Jul 25, 2021

legendecas commented Jul 25, 2021

BethGriggs commented Jul 29, 2021

legendecas commented Jul 29, 2021

legendecas commented Jul 7, 2021 •

edited

Loading

legendecas commented Jul 8, 2021 •

edited

Loading