| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Code changes LGTM
Sorry, something went wrong.
|
We need to add http2 to performanceEntry.entryType. |
Sorry, something went wrong.
|
@hiroppy .... let's do that in a separate PR :-) |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
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/ |
Sorry, something went wrong.
|
Just to be sure, everything one can do with getEntries(), one could do with PerformanceObserver, right? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
|
Tweaked a few things and fixed a bug... new CI: https://ci.nodejs.org/job/node-test-pull-request/13998/ |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@jasnell I have nothing specific against removing them I just thought it would be nice to let them know beforehand |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
New CI is looking good. There's one build bot that's lagging currently but there are no indications that this will fail there. |
Sorry, something went wrong.
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>
|
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. |
Sorry, something went wrong.
Replacement for performance.getEntriesIf 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
} |
Sorry, something went wrong.
|
Is anyone able to summarize why performance.clearMeasures was removed but performance.clearMarks wasn't? |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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