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

perf_hooks: add resourcetiming buffer limit by legendecas · Pull Request #44220 · nodejs/node · GitHub

/ node Public

perf_hooks: add resourcetiming buffer limit - #44220

Closed
legendecas wants to merge 2 commits into
nodejs:mainfrom
legendecas:resource-timing-buffer-limit
ConversationCommits (2)ChecksFiles changed
Closed

perf_hooks: add resourcetiming buffer limit#44220
legendecas wants to merge 2 commits into
nodejs:mainfrom
legendecas:resource-timing-buffer-limit

Conversation

legendecas commented Aug 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

nodejs-github-bot commented Aug 12, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 12, 2022
legendecas force-pushed the resource-timing-buffer-limit branch 2 times, most recently from 601bfad to 2579888 Compare August 12, 2022 06:41

legendecas commented Aug 14, 2022

Copy link
Copy Markdown
Member Author

@nodejs/diagnostics would you mind taking a look at this PR? thank you!

Qard reviewed Aug 14, 2022
View reviewed changes
Comment thread lib/internal/perf/observe.js Outdated

Qard Aug 14, 2022

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

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

legendecas Aug 14, 2022

Copy link
Copy Markdown
Member Author

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

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!

legendecas Aug 15, 2022

Copy link
Copy Markdown
Member Author

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

Updated with aligning the behavior with https://www.w3.org/TR/resource-timing-2/#dfn-add-a-performanceresourcetiming-entry.

legendecas marked this pull request as draft August 14, 2022 16:12
legendecas force-pushed the resource-timing-buffer-limit branch from 2579888 to 6ef3a93 Compare August 15, 2022 08:25
perf_hooks: add resourcetiming buffer limit
e5c6de0 
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.
legendecas force-pushed the resource-timing-buffer-limit branch from 6ef3a93 to e5c6de0 Compare August 15, 2022 08:30
legendecas marked this pull request as ready for review August 15, 2022 08:32
legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 15, 2022

nodejs-github-bot commented Aug 15, 2022 •
edited by legendecas
Loading

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 15, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Aug 16, 2022
Open
9 tasks
RafaelGSS approved these changes Aug 16, 2022
View reviewed changes

RafaelGSS 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 (just a documentation suggestion).

Comment thread doc/api/perf_hooks.md
This was referenced Aug 17, 2022
Open
Open
legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022

legendecas commented Aug 18, 2022

Copy link
Copy Markdown
Member Author

@Qard would you mind taking a look at this again? thank you.

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2022

nodejs-github-bot commented Aug 18, 2022

Copy link
Copy Markdown
Collaborator

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

This was referenced Aug 19, 2022
Open
Open
Open
legendecas added a commit that referenced this pull request Aug 21, 2022
perf_hooks: add resourcetiming buffer limit
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>

legendecas commented Aug 21, 2022

Copy link
Copy Markdown
Member Author

Landed in 798a6ed

legendecas closed this Aug 21, 2022
legendecas deleted the resource-timing-buffer-limit branch August 21, 2022 02:44
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
perf_hooks: add resourcetiming buffer limit
0e38fba 
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>
ruyadorno mentioned this pull request Aug 23, 2022
Merged

juanarbol commented Sep 30, 2022

Copy link
Copy Markdown
Member

This depends on #40532; marked as "dont-land-on-v16.x"

skyclouds2001 mentioned this pull request Nov 8, 2024
Merged
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Qard Qard left review comments
RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL