perf_hooks: implementation of Performance Timing API by jasnell · Pull Request #14680 · nodejs/node

jasnell · 2017-08-07T23:14:43Z

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration.

const { performance } = require('perf_hooks');
performance.mark('A');
setTimeout(() => {
  performance.mark('B');
  performance.measure('A to B', 'A', 'B');
  const entry = performance.getEntriesByName('A to B', 'measure');
  console.log(entry.duration);
}, 10000);

The implementation is at the native layer and makes use of uv_hrtime().
This should enable eventual integeration with the Inspector protocol
so that the performance timeline can be viewed in debugging tools.

The implementation is extensible and should allow us to add new
performance entry types as we go (e.g. for measuring i/o perf,
etc).

Documentation and a test are provided.

/cc @mcollina

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

Trott · 2017-08-07T23:21:45Z

Suggestion: Run make coverage and be sure that all the new code paths are covered by tests.

mscdex · 2017-08-07T23:22:21Z

Why deviate from browsers by putting this on process instead of making it "global" like we do with setTimeout, setInterval, etc.?

addaleax

I’ll try to give this a full review later :)

addaleax · 2017-08-07T23:18:46Z

doc/api/process.md

Starting here, these comments have a broken start token

addaleax · 2017-08-07T23:26:43Z

lib/internal/performance.js

Should this throw?

Refs: http://eslint.org/docs/rules/no-useless-constructor

No, it shouldn't. The constructor is not exposed to end users and those tho get there via process.performance.constructor won't hurt anything if they happen to create a new instance.

jasnell · 2017-08-07T23:45:08Z

@mscdex ... every time I suggest adding a global, people say no.

jasnell · 2017-08-07T23:45:26Z

@Trott ... I will be :-)

mscdex · 2017-08-08T00:01:00Z

@jasnell That might be the case in general, but for APIs shared with browsers it makes more sense to me IMHO. I can't imagine there would be that many people negatively affected by adding it as a "global" (real or otherwise).

jasnell · 2017-08-08T00:02:23Z

@mscdex ... I don't disagree, but I've run into opposition for URL, TextEncoder and TextDecoder, all of which are globals in the browser.

cjihrig · 2017-08-07T23:50:12Z

test/parallel/test-performance.js

The next line makes this assertion unnecessary.

cjihrig · 2017-08-07T23:53:02Z

test/parallel/test-performance.js

You don't need to hoist n.

cjihrig · 2017-08-07T23:59:58Z

src/node.cc

Unrelated change.

cjihrig · 2017-08-08T00:00:20Z

src/node.cc

Unrelated change.

cjihrig · 2017-08-08T00:00:33Z

src/node.cc

Unrelated change.

vsemozhetbyt · 2017-08-08T00:12:34Z

doc/api/process.md

It seems this should go before performance.getEntriesByType(type) alphabetically.

vsemozhetbyt · 2017-08-08T00:15:19Z

doc/api/process.md

These three references are out of the sorting order.

vsemozhetbyt · 2017-08-08T00:25:51Z

doc/api/process.md

is a subclass of PerformanceEntry?

vsemozhetbyt · 2017-08-08T00:33:20Z

doc/api/process.md

The the -> If the?

refack · 2017-08-08T03:15:22Z

Hooking it on process seems strange... IMHO either set it as global or make it a module.

TimothyGu

Code review will come later...

TimothyGu · 2017-08-08T03:40:22Z

doc/api/process.md

It's entryType in the spec...

TimothyGu · 2017-08-08T03:41:18Z

doc/api/process.md

@domenic Should we link to the published version of the spec or the latest editor's draft?

Also this should reference User Timing and High Resolution Time specs since we also implement those.

Always the latest ED

Intentionally started with the first version, will be updating to the latest. The plan is to update incrementally.

TimothyGu · 2017-08-08T03:43:37Z

doc/api/process.md

In Level 2 there's also a toJSON() method in PerformanceEntry that just serializes the four properties.

TimothyGu · 2017-08-08T03:44:47Z

doc/api/process.md

High Resolution Time also defines a toJSON() method.

jasnell · 2017-08-08T05:07:22Z

A couple of things that still need to be done (in case anyone wants to help ;-) ...)

Implement PerformanceObserver .. I've got a couple of ideas on this already so may get to it as early as tomorrow.
Ensure that the list of PerformanceEntry instances are in proper chronological order. PerformanceMeasure instances in particular.
Implement toJSON() on PerformanceEntry

vsemozhetbyt · 2017-08-08T20:41:30Z

doc/api/process.md

I am not sure, but maybe it is worth documenting the callback's arguments? Is observer the same as the new process.PerformanceObserver() here? And maybe PerformanceObserverEntryList API is also worth documenting?

Yeah, i'm going to be updating to include those details :-)

vsemozhetbyt · 2017-08-08T20:47:53Z

doc/api/process.md

It may be my mediocre English, but "instances should be left subscribed to notifications indefinitely" and "Users should disconnect observers as soon as they are no longer needed" seem contradictory. Should it be "should not be left"?

I just forgot the not in should not

jasnell · 2017-08-08T21:51:55Z

Putting this in a new core module makes sense. Currently, the perf_hooks module name is not taken on npm and has good alignment with async_hooks. More obvious other options like performance and perf are already taken.

jasnell · 2017-08-09T00:25:05Z

Ok, I've refactored this to use a new perf_hooks module...

e.g.

const {
  performance,
  PerformanceObserver
} = require('perf_hooks');

performance.mark('test');

thefourtheye · 2017-08-09T01:04:54Z

doc/api/perf_hooks.md

This is the first occurrence of timeOrigin in this file. I think it is better to provide link here.

thefourtheye · 2017-08-09T01:07:08Z

doc/api/perf_hooks.md

If the endMark is not passed, shouldn't we throw an error instead of stringifying an undefined and using it?

We do throw.

thefourtheye · 2017-08-09T01:08:18Z

doc/api/perf_hooks.md

thefourtheye · 2017-08-09T01:09:16Z

doc/api/perf_hooks.md

In few other places in the documentation, high resolution is hyphenated. Perhaps we should unhyphenate them?

thefourtheye · 2017-08-09T01:09:57Z

doc/api/perf_hooks.md

Did you mean Currently it may be?

thefourtheye · 2017-08-09T01:15:34Z

doc/api/perf_hooks.md

thefourtheye · 2017-08-09T01:15:58Z

doc/api/perf_hooks.md

setImmediate

thefourtheye · 2017-08-09T01:16:51Z

doc/api/process.md

Unrelated change?

thefourtheye · 2017-08-09T01:28:50Z

doc/api/perf_hooks.md

If the same name is used, it resets, right? That has to be captured here.

@domenic ... one point of clarification on this. If I call performance.mark('A') twice, should there be two mark entries in the timeline returned by GetEntries() or only one?

As far as I can tell from skimming https://w3c.github.io/user-timing/#dom-performance-mark it creates two entries. Maybe @igrigorik can confirm.

thefourtheye · 2017-08-09T01:31:08Z

src/node_perf.h

Nit: This Node.js feels redundant

mcollina

LGTM with some nits.

mcollina · 2017-08-09T07:21:54Z

lib/perf_hooks.js

Last time I checked, Array.from was extremely slow. Can we do without?

the response has to be an array. I'll be looking at this more tho

mcollina · 2017-08-09T07:24:42Z

lib/perf_hooks.js

why are we using a Set? Typically iterating over an array is way faster, and overhead matters here.

The set is to ensure that items are only added once. I'm looking in to see if we need to keep that requirement.

mcollina · 2017-08-09T07:25:08Z

lib/perf_hooks.js

why setImmediate and not nextTick? Can you add a comment?

The purpose is to allow multiple items to collect. nextTick fires too quickly for that purpose

mcollina · 2017-08-09T07:27:48Z

lib/perf_hooks.js

I usually prefer to have top-level functions for singletons, as the module is already a singleton, but feel free to ignore.

do you mean having module.exports = new Performance() instead? The challenge with that is the PerformanceObserver class, which also hangs off module.exports here.

An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, garbage collection timing, and node startup milestone timing. ```js const { performance } = require('perf_hooks'); performance.mark('A'); setTimeout(() => { performance.mark('B'); performance.measure('A to B', 'A', 'B'); const entry = performance.getEntriesByName('A to B', 'measure')[0]; console.log(entry.duration); }, 10000); ``` The implementation is at the native layer and makes use of uv_hrtime(). This should enable *eventual* integration with things like Tracing and Inspection. The implementation is extensible and should allow us to add new performance entry types as we go (e.g. for measuring i/o perf, etc). Documentation and a test are provided. PR-URL: #14680 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

sam-github · 2017-09-22T21:42:28Z

doc/api/perf_hooks.md

+`PerformanceNodeTiming` class. If the named `endMark` does not exist, an
+error will be thrown.
+
+### performance.nodeFrame


@jasnell I'm not finding this property on the performance object, and I can't find any sign of a PerformanceFrame or a PerformanceNodeFrame classes anywhere other than the docs. Did this not get implemented because it depends on libuv/libuv#1489, or is there something else I am missing?

That should have been removed. I may have forgotten to do so. Yes, the libuv pr needs to land before that can return

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs#14680 (comment)

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: #14680 (comment) PR-URL: #15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs/node#14680 (comment) PR-URL: nodejs/node#15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: #14680 (comment) PR-URL: #15641 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

gibfahn · 2018-01-15T22:11:58Z

Release team were -1 on landing on v6.x, if you disagree let us know.

fanatid · 2019-09-03T17:02:42Z

Sorry if this wrong place for ask, but I'm not sure where will be better.
I'm going through perf_hooks and can not find where entryType equal to node used. Is it used at all? If not, why this was added?
https://github.com/fanatid/notes/tree/master/2019-09-12-node-perf-hooks

PR-URL: #55247 Refs: #14680 Refs: #39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#55247 Refs: nodejs#14680 Refs: nodejs#39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 7, 2017

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 7, 2017

jasnell requested review from addaleax and mcollina August 7, 2017 23:15

jasnell force-pushed the performance-api branch from 5bc0b95 to 8a059b9 Compare August 7, 2017 23:21

vsemozhetbyt added the performance Issues and PRs related to the performance of Node.js. label Aug 7, 2017

addaleax reviewed Aug 7, 2017

View reviewed changes

cjihrig reviewed Aug 8, 2017

View reviewed changes

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

doc/api/process.md Outdated

Copy link

Contributor

vsemozhetbyt Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three references are out of the sorting order.

jasnell reacted with thumbs up emoji

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

doc/api/process.md Outdated

Copy link

Contributor

vsemozhetbyt Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a subclass of PerformanceEntry?

jasnell reacted with thumbs up emoji

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

doc/api/process.md Outdated

Copy link

Contributor

vsemozhetbyt Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The the -> If the?

jasnell reacted with thumbs up emoji

TimothyGu reviewed Aug 8, 2017

View reviewed changes

hiroppy added the process Issues and PRs related to the process subsystem. label Aug 8, 2017

benjamingr mentioned this pull request Aug 8, 2017

performance should work in Node sinonjs/fake-timers#128

Closed

vsemozhetbyt reviewed Aug 8, 2017

View reviewed changes

jasnell force-pushed the performance-api branch from e33f50c to 7ddf8a8 Compare August 9, 2017 00:23

thefourtheye reviewed Aug 9, 2017

View reviewed changes

mcollina approved these changes Aug 9, 2017

View reviewed changes

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

sam-github reviewed Sep 22, 2017

View reviewed changes

sam-github added a commit to sam-github/node that referenced this pull request Sep 27, 2017

perf_hooks: remove docs for unimplemented API

05e6a59

The node frame (aka loop) timing API did not land, it depends on libuv/libuv#1489 which is still a WIP. See: nodejs#14680 (comment)

sam-github mentioned this pull request Sep 27, 2017

perf_hooks: remove docs for unimplemented API #15641

Closed

4 tasks

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

antonyt mentioned this pull request Jan 23, 2018

doc: Add doc for performance.clearGC() #18331

Closed

2 tasks

Trott mentioned this pull request Oct 6, 2018

Investigate flaky sequential/test-performance #23291

Closed

aduh95 mentioned this pull request Oct 3, 2024

doc: move ERR_INVALID_PERFORMANCE_MARK to legacy errors #55247

Merged

nodejs-github-bot pushed a commit that referenced this pull request Oct 10, 2024

doc: move ERR_INVALID_PERFORMANCE_MARK to legacy errors

4a3fffa

PR-URL: #55247 Refs: #14680 Refs: #39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

aduh95 added a commit that referenced this pull request Oct 11, 2024

doc: move ERR_INVALID_PERFORMANCE_MARK to legacy errors

d8e42be

PR-URL: #55247 Refs: #14680 Refs: #39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024

doc: move ERR_INVALID_PERFORMANCE_MARK to legacy errors

557e54c

PR-URL: nodejs#55247 Refs: nodejs#14680 Refs: nodejs#39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

skyclouds2001 mentioned this pull request Nov 8, 2024

Update nodejs data for performance global property mdn/browser-compat-data#25006

Merged

tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024

doc: move ERR_INVALID_PERFORMANCE_MARK to legacy errors

955bfbf

PR-URL: nodejs#55247 Refs: nodejs#14680 Refs: nodejs#39297 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Uh oh!

Conversation

jasnell commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Aug 7, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 7, 2017

Uh oh!

jasnell commented Aug 7, 2017

Uh oh!

mscdex commented Aug 8, 2017

Uh oh!

jasnell commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 8, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 8, 2017

Uh oh!

jasnell commented Aug 7, 2017 •

edited

Loading

Trott commented Aug 7, 2017 •

edited

Loading

jasnell Aug 8, 2017 •

edited

Loading