perf_hooks: add warning when too many entries in the timeline by jasnell · Pull Request #18087 · nodejs/node

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

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

Uh oh!

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 wants to merge 3 commits intonodejs:masterfrom
jasnell:perf-warning

jasnell commented Jan 10, 2018

Uh oh!

Fishrock123 Jan 10, 2018

Uh oh!

jasnell Jan 10, 2018

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

BridgeAR Jan 24, 2018

Uh oh!

jasnell Jan 24, 2018

Uh oh!

mcollina left a comment

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

jasnell commented Jan 10, 2018

Checklist

Affected core subsystem(s)

Uh oh!

Fishrock123 Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 10, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 24, 2018

Uh oh!

BridgeAR Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

jasnell commented Jan 31, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants