FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf_hooks: simplify perf_hooks by jasnell · Pull Request #19563 · nodejs/node · GitHub

/ node Public

perf_hooks: simplify perf_hooks - #19563

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:simplify-perf-hooks
Closed

perf_hooks: simplify perf_hooks#19563
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:simplify-perf-hooks

Conversation

jasnell commented Mar 23, 2018

Copy link
Copy Markdown
Member

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Mar 23, 2018

jasnell commented Mar 23, 2018

Copy link
Copy Markdown
Member Author

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code changes LGTM

hiroppy commented Mar 23, 2018

Copy link
Copy Markdown
Member

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

jasnell commented Mar 24, 2018

Copy link
Copy Markdown
Member Author

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

jasnell force-pushed the simplify-perf-hooks branch from 00932a3 to 229fed3 Compare March 24, 2018 15:00

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Copy link
Copy Markdown

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

lrlna commented Mar 25, 2018

Copy link
Copy Markdown
Contributor

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/

Copy link
Copy Markdown
Member

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

jasnell commented Mar 25, 2018

Copy link
Copy Markdown
Member Author

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.

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.
jasnell force-pushed the simplify-perf-hooks branch from 229fed3 to 9f9ff4c Compare April 2, 2018 18:58

jasnell commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

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

devsnek commented Apr 2, 2018
edited
Loading

Copy link
Copy Markdown
Member

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 commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

@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 commented Apr 2, 2018

Copy link
Copy Markdown
Member

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

jasnell commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

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 added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 2, 2018

jasnell commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

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 added a commit that referenced this pull request Apr 3, 2018
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.

PR-URL: #19563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell commented Apr 3, 2018

Copy link
Copy Markdown
Member Author

Landed in 2ec6995

jasnell closed this Apr 3, 2018

targos commented Apr 4, 2018

Copy link
Copy Markdown
Member

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 commented Jan 7, 2019
edited
Loading

Copy link
Copy Markdown

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 commented May 7, 2020

Copy link
Copy Markdown
Contributor

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

jasnell commented May 7, 2020

Copy link
Copy Markdown
Member Author

@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 commented May 8, 2020

Copy link
Copy Markdown
Contributor

@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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. notable-change PRs with changes that should be highlighted in changelogs. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.


Back | FazBrowse Home | New Git URL