perf_hooks: simplify perf_hooks

[jasnell]

While perf_hooksare still experimental, remove the performance.getEntries() and performance.clear*() variants and eliminate the accumulation of the global timeline entries. The design of this particular bit of the API is a memory leak and performance footgun. The PerformanceObserver API is a better approach to consuming the data in a more transient way.

Note that this does mean having a slight variance with the browser based API but it's an important one. Even the browser API suffers from the possibility of a memory leak the way the performance timeline API is defined. Any other attempt to mitigate this would also mean deviating from the standard API definition so let's just do it right and remove the problematic bits while we can.

/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

[jasnell]

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

[addaleax]

Code changes LGTM

[hiroppy]

We need to add http2 to performanceEntry.entryType.
https://github.com/nodejs/node/blob/master/doc/api/perf_hooks.md#performanceentryentrytype

[jasnell]

@hiroppy .... let's do that in a separate PR :-)

[mcollina]

LGTM

[yoshuawuyts]

Even the browser API suffers from the possibility of a memory leak the way the performance timeline API is defined.

It's probably important to know that after the performance timeline has filled up, it's no longer possible to create events - which means the memory leak is quite contained.

I'm +1 on this change though. Continuously clearing timings without having a way to clear single timings is annoying to say the least. And from the looks of it this will simply make using this API faster.

Real neat!

Refs

• https://github.com/choojs/on-performance
• https://github.com/choojs/nanotiming

[lrlna]

This looks great, @jasnell! It makes a lot more sense to get people to just work with the observer API instead. As in, you might start of with performance.getEntries(), but probably going to move on the observer anyways since it's much more useful to you o/ o/ o/

[TimothyGu]

Just to be sure, everything one can do with getEntries(), one could do with PerformanceObserver, right?

[jasnell]

Yep, there's just no persistence of the performance entries so you'll need to have a PerformanceObserver registered in advance to catch the entries and work with them.

[jasnell]

Tweaked a few things and fixed a bug... new CI: https://ci.nodejs.org/job/node-test-pull-request/13998/

[devsnek]

a lot of the stuff removed here appears to be specified by w3c, have we properly considered this/reached out to the web performance wg?

[jasnell]

@devsnek ... that's actually part of the point. The bits removed here are from a part of the defined standard API that is not super usable, not super useful, and has a non-zero risk of introducing memory leaks. It is something that can easily be re-implemented by userland code if necessary.

[devsnek]

@jasnell I have nothing specific against removing them I just thought it would be nice to let them know beforehand

[jasnell]

Yeah, I've already put out a heads up on my twitter feed. Both @yoshuawuyts and @lrlna are consumers of the API, and this would go into the notable changes for whatever release it lands in. The feature is still marked as experimental as well.

[jasnell]

New CI is looking good. There's one build bot that's lagging currently but there are no indications that this will fail there.

[jasnell]

Landed in 2ec6995

[targos]

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

[vjpr]

Replacement for performance.getEntries

If you want the same functionality just use something like this:

const measures = []
const obs = new PerformanceObserver(list => {
  measures.push(...list.getEntries())
  obs.disconnect()
})
obs.observe({entryTypes: ['measure']})
function getEntriesByType(name) {
  return measures
}

[jmm]

Is anyone able to summarize why performance.clearMeasures was removed but performance.clearMarks wasn't?

[jasnell]

@jmm ... Yes, the challenge here is that the Performance API as specified actually includes a bit of a built in memory leak (e.g. things like measures accumulate forever until the user explicitly clears them). We have to retain marks simply because of their nature but we handle measures as being fully transient (that is, measures are not persisted after they are emitted to a PerformanceObserver). This avoids requiring users to manually clear out old performance entries.

[jmm]

@jasnell Thanks!

That's the gist I understood from reading the previous posts, but I was confused because in AWS Lambda it seemed I was getting accumulating measures each time the container was re-used. After your reply I realized that it wasn't accumulating measures, it was my mistake accumulating PerformanceObservers because I wasn't disconnecting them. I changed it to disconnect the observers and clear marks after taking a measure for...good measure.

So, sorry to make you repeat yourself :), but I definitely appreciate the reply.