Skip to content

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

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-next-tick-exec-adjust
Closed

benchmark: track exec time in next-tick-exec#20462
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-next-tick-exec-adjust

Conversation

@apapirovski

@apapirovskiapapirovski commented May 2, 2018

Copy link
Copy Markdown
Contributor

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.
@apapirovskiapapirovski added the benchmark Issues and PRs related to the benchmark subsystem. label May 2, 2018
@nodejs-github-botnodejs-github-bot added the process Issues and PRs related to the process subsystem. label May 2, 2018
@apapirovski

apapirovski commented May 2, 2018

Copy link
Copy Markdown
ContributorAuthor

CI Lite: https://ci.nodejs.org/job/node-test-pull-request-lite/647/

@BridgeAR

BridgeAR commented May 2, 2018

Copy link
Copy Markdown
Member

Shall we maybe add an extra benchmark for this instead? Or do we have those already?

@apapirovski

apapirovski commented May 2, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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

apapirovski commented May 2, 2018

Copy link
Copy Markdown
ContributorAuthor

I would like to fast track this so I can run the benchmarks in #20468. Please 👍 here to approve.

@apapirovskiapapirovski 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

@jasnelljasnell 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.

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

LGTM but would like to get more review :-)

@apapirovski

apapirovski commented May 4, 2018

Copy link
Copy Markdown
ContributorAuthor

@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

apapirovski commented May 6, 2018

Copy link
Copy Markdown
ContributorAuthor

Landed in 34bd9f3

@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>
@MylesBorinsMylesBorins 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@TrottTrottTrott approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@trivikrtrivikrtrivikr approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.benchmarkIssues and PRs related to the benchmark subsystem.processIssues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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