perf_hooks: add resourcetiming buffer limit#44220
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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
Collaborator
nodejs-github-bot commented Aug 12, 2022
Review requested:
|
nodejs-github-bot added lib / src 
legendecasforce-pushed the resource-timing-buffer-limit branch 2 times, most recently from 601bfad to 2579888Compare
MemberAuthor
legendecas commented Aug 14, 2022
@nodejs/diagnostics would you mind taking a look at this PR? thank you! |
Qard reviewed Aug 14, 2022
lib/internal/perf/observe.js Outdated
| */ | ||
| functionbufferResourceTiming(entry){ | ||
| if(resourceTimingBuffer.length>=resourceTimingBufferSizeLimit){ | ||
| dispatchBufferFull('resourcetimingbufferfull'); |
Member
There was a problem hiding this comment.
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. 🤔
MemberAuthor
There was a problem hiding this comment.
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!
MemberAuthor
There was a problem hiding this comment.
Updated with aligning the behavior with https://www.w3.org/TR/resource-timing-2/#dfn-add-a-performanceresourcetiming-entry.
legendecasforce-pushed the resource-timing-buffer-limit branch from 2579888 to 6ef3a93Compare
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.
legendecasforce-pushed the resource-timing-buffer-limit branch from 6ef3a93 to e5c6de0Compare
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Aug 15, 2022
9 tasks
RafaelGSS approved these changes Aug 16, 2022
Member
RafaelGSS left a comment
RafaelGSS left a comment There was a problem hiding this comment.
LGTM (just a documentation suggestion).
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 17, 2022
MemberAuthor
legendecas commented Aug 18, 2022
@Qard would you mind taking a look at this again? thank you. |
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Aug 18, 2022
This was referenced Aug 19, 2022
legendecas added a commit that referenced this pull request Aug 21, 2022
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 <[email protected]>
MemberAuthor
legendecas commented Aug 21, 2022
Landed in 798a6ed |
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
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 <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
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 <[email protected]>
Member
juanarbol commented Sep 30, 2022
This depends on #40532; marked as "dont-land-on-v16.x" |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add WebPerf API
performance.setResourceTimingBufferSizeand 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.