Skip to content
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: reduce overhead of new resource timings #49837

Merged
merged 2 commits into from
Sep 30, 2023

Conversation

H4ad
Copy link
Member

@H4ad H4ad commented Sep 24, 2023

Continuing the work started on nodejs/performance#109.

This PR should be landed after #49803.

                                                         confidence improvement accuracy (*)    (**)   (***)
perf_hooks/resourcetiming.js observe='resource' n=100000        ***   1086.23 %      ±40.80% ±54.98% ±72.99%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

I want to use the symbol kSkipThrow in other classes around the NodeJS codebase, someone had some idea where I should put it? Or create a new symbol and reuse it when possible?

Right now, we have this symbol being used on perf_hooks but there is also an identical symbol on webstreams, so maybe we can put it in a higher module and import it around the node, I initially thought to put it inside errors, since we will always use it together with ERR_ILLEGAL_CONSTRUCTOR.

I will reuse the same symbol only when it inherits some class that exports a symbol, otherwise, I will create a new one.

/cc @nodejs/performance

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 24, 2023
@benjamingr
Copy link
Member

I want to use the symbol kSkipThrow in other classes around the NodeJS codebase, someone had some idea where I should put it? Or create a new symbol and reuse it when possible?

I would use a new symbol for each constructor so that if other parts in the code base need to create something in a way not exposed to userland they need to explicitly import it. We use this pattern in many other places like event_target too.

@H4ad H4ad force-pushed the perf/perf_hooks_resource_timings branch from 42d7511 to cdcfc29 Compare September 28, 2023 00:09
@H4ad H4ad marked this pull request as ready for review September 28, 2023 00:12
@H4ad H4ad requested a review from benjamingr September 28, 2023 00:13
@anonrig anonrig added performance Issues and PRs related to the performance of Node.js. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-bot nodejs-github-bot merged commit e6e320e into nodejs:main Sep 30, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e6e320e

GeoffreyBooth pushed a commit to GeoffreyBooth/node that referenced this pull request Oct 1, 2023
PR-URL: nodejs#49837
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@H4ad H4ad deleted the perf/perf_hooks_resource_timings branch October 1, 2023 13:27
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49837
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49837
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49837
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants