-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf_hooks: add resourcetiming buffer limit #44220
Conversation
Review requested:
|
601bfad
to
2579888
Compare
@nodejs/diagnostics would you mind taking a look at this PR? thank you! |
lib/internal/perf/observe.js
Outdated
*/ | ||
function bufferResourceTiming(entry) { | ||
if (resourceTimingBuffer.length >= resourceTimingBufferSizeLimit) { | ||
dispatchBufferFull('resourcetimingbufferfull'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too familiar with the spec, but is this timing expected? Seems to me like it makes more sense for the event to dispatch immediately after reaching capacity so the buffer can be consumed before the next event is received, not at the point the next event is received and discarding the event without having any chance to react before it is discarded. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems like this is a mismatch with the spec. I'll check again on the steps with the spec conformance. Thanks for pointing this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with aligning the behavior with https://www.w3.org/TR/resource-timing-2/#dfn-add-a-performanceresourcetiming-entry.
2579888
to
6ef3a93
Compare
Add WebPerf API `performance.setResourceTimingBufferSize` and event `'resourcetimingbufferfull'` support. The resource timing entries are added to the global performance timeline buffer automatically when using fetch. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely.
6ef3a93
to
e5c6de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (just a documentation suggestion).
@Qard would you mind taking a look at this again? thank you. |
Add WebPerf API `performance.setResourceTimingBufferSize` and event `'resourcetimingbufferfull'` support. The resource timing entries are added to the global performance timeline buffer automatically when using fetch. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely. PR-URL: #44220 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Landed in 798a6ed |
Add WebPerf API `performance.setResourceTimingBufferSize` and event `'resourcetimingbufferfull'` support. The resource timing entries are added to the global performance timeline buffer automatically when using fetch. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely. PR-URL: #44220 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Add WebPerf API `performance.setResourceTimingBufferSize` and event `'resourcetimingbufferfull'` support. The resource timing entries are added to the global performance timeline buffer automatically when using fetch. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely. PR-URL: nodejs#44220 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
This depends on #40532; marked as "dont-land-on-v16.x" |
Add WebPerf API
performance.setResourceTimingBufferSize
and event'resourcetimingbufferfull'
support.The resource timing entries are added to the global performance timeline buffer automatically when using
fetch
. If users are not proactively cleaning these events, it can grow without limit. Apply the https://www.w3.org/TR/timing-entrytypes-registry/ default resource timing buffer max size so that the buffer can be limited to not grow indefinitely.