perf_hooks: add warning when too many entries in the timeline #18087

jasnell · 2018-01-10T17:43:54Z

Due to quirks in how the performance API is defined, there is a non-zero risk of memory leaks. Add an upper limit and emit a warning if it is reached.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

perf_hooks

Fishrock123 · 2018-01-10T23:21:47Z

doc/api/perf_hooks.md

@@ -109,6 +109,18 @@ Creates a new `PerformanceMark` entry in the Performance Timeline. A
 `performanceEntry.duration` is always `0`. Performance marks are used
 to mark specific significant moments in the Performance Timeline.

+### performance.maxBufferSize


By the description, I think the property name might be better as something more related?

Something like maxEntries? Unless it needs to expose the detail about being a buffer?

I'm all for a better name. The current was chosen to have some resemblance to https://developer.mozilla.org/en-US/docs/Web/API/Performance/setResourceTimingBufferSize

jasnell · 2018-01-24T18:06:17Z

@Fishrock123 ... renamed

@mcollina and @BridgeAR ... can I ask the two of you to take a quick look?

BridgeAR · 2018-01-24T18:36:48Z

doc/api/perf_hooks.md

+added: REPLACEME
+-->
+
+Value: {number}


There is a default of 150 set. It should be documented as well.

mcollina

LGTM. How was 150 picked?

jasnell · 2018-01-31T20:04:46Z

150 is largely arbitrary and can be tweaked if necessary.

jasnell · 2018-01-31T22:25:49Z

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

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2018-01-31T23:26:59Z

Landed in 6bcd31f

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins · 2018-03-20T15:51:07Z

seems like maybe this should have been semver-minor

LMK if I am mistaken

PR-URL: nodejs#18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Jan 10, 2018

jasnell force-pushed the perf-warning branch from acc31bb to f6b4028 Compare January 10, 2018 17:46

Fishrock123 reviewed Jan 10, 2018

View reviewed changes

jasnell added 2 commits January 24, 2018 10:04

perf_hooks: add warning when too many entries in the timeline

fa7e500

[Squash] Rename

3227a91

jasnell force-pushed the perf-warning branch from f6b4028 to 3227a91 Compare January 24, 2018 18:05

BridgeAR reviewed Jan 24, 2018

View reviewed changes

[Squash] Include default

75771f6

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

mcollina approved these changes Jan 30, 2018

View reviewed changes

jasnell added a commit that referenced this pull request Jan 31, 2018

perf_hooks: add warning when too many entries in the timeline

6bcd31f

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell closed this Jan 31, 2018

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

perf_hooks: add warning when too many entries in the timeline

0915a85

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

perf_hooks: add warning when too many entries in the timeline

75975c7

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

perf_hooks: add warning when too many entries in the timeline

0a47b98

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 20, 2018

MylesBorins added dont-land-on-v6.x labels Mar 20, 2018

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

perf_hooks: add warning when too many entries in the timeline

52a32d0

PR-URL: nodejs#18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins added the land-on-v8.x label Aug 2, 2018

MylesBorins removed the lts-watch-v8.x label Aug 2, 2018

MylesBorins pushed a commit that referenced this pull request Aug 2, 2018

perf_hooks: add warning when too many entries in the timeline

a94c726

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 2, 2018

perf_hooks: add warning when too many entries in the timeline

33e300c

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

rvagg pushed a commit that referenced this pull request Aug 16, 2018

perf_hooks: add warning when too many entries in the timeline

b4192b0

PR-URL: #18087 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf_hooks: add warning when too many entries in the timeline #18087

perf_hooks: add warning when too many entries in the timeline #18087

jasnell commented Jan 10, 2018

Fishrock123 Jan 10, 2018

jasnell Jan 10, 2018

jasnell commented Jan 24, 2018

BridgeAR Jan 24, 2018

jasnell Jan 24, 2018

mcollina left a comment

jasnell commented Jan 31, 2018

jasnell commented Jan 31, 2018

jasnell commented Jan 31, 2018

MylesBorins commented Mar 20, 2018

perf_hooks: add warning when too many entries in the timeline #18087

perf_hooks: add warning when too many entries in the timeline #18087

Conversation

jasnell commented Jan 10, 2018

Checklist

Affected core subsystem(s)

Fishrock123 Jan 10, 2018

Choose a reason for hiding this comment

jasnell Jan 10, 2018

Choose a reason for hiding this comment

jasnell commented Jan 24, 2018

BridgeAR Jan 24, 2018

Choose a reason for hiding this comment

jasnell Jan 24, 2018

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

jasnell commented Jan 31, 2018

jasnell commented Jan 31, 2018

jasnell commented Jan 31, 2018

MylesBorins commented Mar 20, 2018