benchmarks: improve code base by BridgeAR · Pull Request #18320 · nodejs/node · GitHub

Just some refactoring of some benchmarks to reduce code overhead and improve readability.

I some times changed let in a loop to var because it may theoretically still lead to a deopt / prevent a opt.

I fixed two benchmarks that regressed before and what I realized when going over all files again.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

benchmark

LGTM with some nits

This doesn't look correct? For the not versions it needs to use expectedWrong, right?

You are absolutely correct. While looking over the file I missed that.

Could this be more similar to the change above but just assign expected/expectedWrong to a diff variable as necessary. Then the for loops, etc. don't need to be part of the switch.

Same for all of the similar use cases below.

Done

I don't think this change is necessary since the let is not declared within the for loop and for general usage it was optimized quite a while ago.

I don't think this change is quite right. It means the Buffer is always the first element of strings even though it shouldn't be. I would prefer to leave this benchmark as is. The only change I can see is not allocating the empty array in strings and instead allocating it within the else branch.

millions * 1e6 will happen on every iteration of the loop. I would prefer it assigned to a const.

I did not check but I am pretty certain that the compiler creates a constant internally after evaluating the computation.

I know that's sort of the case with length but I wonder in this case... ping @bmeurer

TurboFan should sort that out, yes.

@apapirovski I personally would like to get rid of "millions" and use "n" instead. But for now I would just stick to the way I changed it.

But I am also fine with removing that change again as it is not important at all. What do you prefer?

Same as above. There are a few more of these in the files below too...

Nice catch.

Can we store the array in a variable in addition to this change? The results here are probably dominated by the array creation.

@apapirovski PTAL

CI https://ci.nodejs.org/job/node-test-pull-request/12708/

@BridgeAR looks like one of the assert benchmarks is broken, or if not then at the very least the test that runs them.

Fixed

New CI: https://ci.nodejs.org/job/node-test-pull-request/12766/

It should say `win32` and not `posix`.

Due to the destructuring the outer variables were not set anymore.

Rebased due to conflicts.

New CI https://ci.nodejs.org/job/node-test-pull-request/12872/

It should say `win32` and not `posix`. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

Due to the destructuring the outer variables were not set anymore. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

It should say `win32` and not `posix`. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

Due to the destructuring the outer variables were not set anymore. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

Should this be backported to v8.x-staging or v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@MylesBorins looks like this one's pending a backport? I'll love to give it a shot.

Also, do I still need to backport it to v6? When it landed, v6 wasn't in maintenance, but it is now.

It does not need to be backported to 6. Lmk of you need help with 8 at all

@MylesBorins this looks super tricky because there's a change that landed later (but got backported before), that seems to have refactored almost everything.

For example, take a look at:

in here, which change do I accept? Accepting the current change looks like a nice thing to do but I'll have to manually remove the last 4 lines (is that okay?). Accepting the incoming change won't work because it needs the method variable.

@ryzokuken I would suggest trying to get a result that is closest to what is upstream (in master) (if that makes sense)

P.S. Do I accept both and modify the solutions (mix and match)? That doesn't sound like something that should happen without the consent of the PR owner, given that such a thing might completely go against what they initially intended.

@ryzokuken I would suggest trying to get a result that is closest to what is upstream (if that makes sense)

Only keep stuff from the PR (incoming changes) that has 0 conflicts and is absolutely necessary, you mean?

@MylesBorins there are files that were removed later. Should I re-add them with the changed version?

apapirovski reviewed Jan 23, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR force-pushed the improve-benchmarks branch from 982cf6d to 3f238eb Compare January 27, 2018 02:32

jasnell approved these changes Jan 29, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 30, 2018

BridgeAR added 10 commits February 1, 2018 09:43

benchmark: fix platform in basename-win32

03d5bf4

It should say `win32` and not `posix`.

benchmark: fix variables not being set

42d591c

Due to the destructuring the outer variables were not set anymore.

benchmark: (assert) refactor

d7a387d

benchmark: (buffer) refactor

95b9b09

benchmark: (crypto) refactor

a012a56

benchmark: (url) refactor

d094ba9

benchmark: (es) refactor

3425305

benchmark: (http(2)) refactor

1b8ee8a

benchmark: (timers) refactor

5aa2a2f

benchmark: refactor

f7d9fac

BridgeAR force-pushed the improve-benchmarks branch from 3f238eb to f7d9fac Compare February 1, 2018 08:45

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: fix platform in basename-win32

f023d58

It should say `win32` and not `posix`. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: fix variables not being set

0ac2a9d

Due to the destructuring the outer variables were not set anymore. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (assert) refactor

d20e524

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (buffer) refactor

4f09f08

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (crypto) refactor

3d205a3

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (url) refactor

10a560a

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (es) refactor

5213f8c

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (http(2)) refactor

0d7b697

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: (timers) refactor

bb808e3

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

benchmark: refactor

8aabb86

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

benchmark: fix platform in basename-win32

7d077ea

It should say `win32` and not `posix`. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

benchmark: fix variables not being set

5d680fb

Due to the destructuring the outer variables were not set anymore. PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

benchmark: (assert) refactor

4e3f482

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

benchmark: (buffer) refactor

edbf81a

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

benchmark: (crypto) refactor

084165a

PR-URL: #18320 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

ryzokuken mentioned this pull request Jun 6, 2018

[v8.x backport] benchmarks: improve code base #21169

Closed

4 tasks

benchmarks: improve code base by BridgeAR · Pull Request #18320 · nodejs/node · GitHub

Uh oh!

Uh oh!

benchmarks: improve code base - #18320

benchmarks: improve code base#18320BridgeAR wants to merge 10 commits intonodejs:masternodejs/node:masterfrom BridgeAR:improve-benchmarksBridgeAR/node:improve-benchmarksCopy head branch name to clipboard

Conversation

BridgeAR commented Jan 23, 2018

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jan 24, 2018

Uh oh!

BridgeAR commented Jan 24, 2018

Uh oh!

apapirovski commented Jan 24, 2018

Uh oh!

BridgeAR commented Jan 27, 2018

Uh oh!

BridgeAR commented Feb 1, 2018

Uh oh!

MylesBorins commented Mar 20, 2018

Uh oh!

ryzokuken commented Jun 6, 2018

Uh oh!

MylesBorins commented Jun 6, 2018

Uh oh!

ryzokuken commented Jun 6, 2018

Uh oh!

MylesBorins commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken commented Jun 6, 2018

Uh oh!

ryzokuken commented Jun 6, 2018

Uh oh!

ryzokuken commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

benchmarks: improve code base#18320
BridgeAR wants to merge 10 commits into
nodejs:masterfrom
BridgeAR:improve-benchmarks

MylesBorins commented Jun 6, 2018 •
edited
Loading