-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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 createHistogram #50074
Conversation
@@ -318,26 +327,36 @@ class RecordableHistogram extends Histogram { | |||
} | |||
} | |||
|
|||
function internalHistogram(handle) { | |||
function ClonedHistogram(handle) { |
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.
This ReflectConstruct can be removed as well
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, but since it is not exposed directly I didn't think that was worth it, if not for spec compliance, this code could be removed (I think).
76fb187
to
d58450a
Compare
d58450a
to
84d042c
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, with a minor nit.
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
Someone can add author ready? |
I added another commit to try address https://ci.nodejs.org/job/node-test-commit-linux/nodes=ubuntu1804-64/54333/console, I think now both functions are optimized, it is causing an issue with v8 dead code. |
PR-URL: #50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@H4ad Why this was landed with |
My idea was to ship both commits since I initially thought they should be independent, but thinking more about the reason why I had the commit of But making all the tests pass for each commit that will land in the same PR is something I misread from the guidelines, in case I need to do that, how do I ensure all the tests will pass for previous commits, should I call the job |
Usually, you guarantee by running the tests on your machine. When you add the
commit-queue-rebase
|
I can confirm that the test suite passes on both commits that landed, so no bisect has been harmed :) The guidelines are vague on what constitutes an atomic commit, so it’s a judgement call to make and both decisions would have been correct: it could have landed as one commit, it could have landed as two commits. |
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
@H4ad this commit didn’t land cleanly on |
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: nodejs#50074 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Backport-PR-URL: nodejs#50074
PR-URL: #50074 Backport-PR-URL: #51306 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #50074 Backport-PR-URL: #51306 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #50074 Backport-PR-URL: #51306 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: #50074 Backport-PR-URL: #51306 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Continuing the work started on nodejs/performance#109
/cc @nodejs/performance