Skip to content

Conversation

@H4ad
Copy link
Member

@H4adH4ad 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-botnodejs-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.

@H4adH4adforce-pushed the perf/perf_hooks_resource_timings branch from 42d7511 to cdcfc29CompareSeptember 28, 2023 00:09
@H4adH4ad marked this pull request as ready for review September 28, 2023 00:12
@anonriganonrig 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-actionsgithub-actionsbot 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

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2023
@nodejs-github-botnodejs-github-bot merged commit e6e320e into nodejs:mainSep 30, 2023
@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]>
@H4adH4ad 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]>
@targostargos mentioned this pull request Nov 12, 2023
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@H4ad@benjamingr@nodejs-github-bot@Qard@anonrig@Uzlopak@aduh95