Skip to content

benchmark: track exec time in next-tick-exec #20462

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

benchmark: track exec time in next-tick-exec #20462

wants to merge 1 commit into from

Conversation

@apapirovski
Copy link
Contributor

@apapirovski apapirovski commented May 2, 2018

The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error (on my part... 😆), they actually track addition and execution.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@apapirovski
benchmark: track exec time in next-tick-exec
d355dbf 
The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error, they actually track addition and execution.
@apapirovski apapirovski added the benchmark Issues and PRs related to the benchmark subsystem. label May 2, 2018
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label May 2, 2018
@apapirovski
Copy link
Contributor Author

apapirovski commented May 2, 2018

CI Lite: https://ci.nodejs.org/job/node-test-pull-request-lite/647/
@BridgeAR
Copy link
Member

BridgeAR commented May 2, 2018

Shall we maybe add an extra benchmark for this instead? Or do we have those already?
@apapirovski
Copy link
Contributor Author

apapirovski commented May 2, 2018 •
edited
Loading

@BridgeAR We basically do. next-tick-depth and next-tick-breadth (plus -args versions) should mostly cover it.

I won't stop anyone from adding more benchmarks but these two are even named exec... kinda misleading :)
@apapirovski
Copy link
Contributor Author

apapirovski commented May 2, 2018

I would like to fast track this so I can run the benchmarks in #20468. Please 👍 here to approve.
@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2018
jasnell
jasnell approved these changes May 4, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mscdex ... what do you think of this one?

LGTM but would like to get more review :-)
@apapirovski
Copy link
Contributor Author

apapirovski commented May 4, 2018

@jasnell @mscdex This hopefully shouldn't be particularly controversial. This benchmark, as is, is pretty similar to the breadth one. It was only added for this specific purpose but I made an error in the original PR.

FWIW I'm planning to land this at the end of the 48 hours.
@apapirovski
Copy link
Contributor Author

apapirovski commented May 6, 2018

Landed in 34bd9f3
@apapirovski apapirovski closed this May 6, 2018
@apapirovski apapirovski deleted the patch-next-tick-exec-adjust branch May 6, 2018 05:30
apapirovski added a commit that referenced this pull request May 6, 2018
@apapirovski
benchmark: track exec time in next-tick-exec
34bd9f3 
The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error, they actually track addition and execution. PR-URL: #20462 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@apapirovski @MylesBorins
benchmark: track exec time in next-tick-exec
d0d0547 
The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error, they actually track addition and execution. PR-URL: #20462 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 8, 2018
@apapirovski @MylesBorins
benchmark: track exec time in next-tick-exec
55af327 
The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error, they actually track addition and execution. PR-URL: #20462 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 9, 2018
@apapirovski @MylesBorins
benchmark: track exec time in next-tick-exec
16970ff 
The next-tick-exec benchmarks were meant to track nextTick execution time but due to an error, they actually track addition and execution. PR-URL: #20462 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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. benchmark Issues and PRs related to the benchmark subsystem. process Issues and PRs related to the process subsystem.

6 participants

@apapirovski @BridgeAR @jasnell @Trott @trivikr @nodejs-github-bot