benchmarks: improve code base #18320

BridgeAR · 2018-01-23T13:45:25Z

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

apapirovski

LGTM with some nits

apapirovski · 2018-01-23T19:05:38Z

benchmark/assert/deepequal-buffer.js

+
+  bench.start();
+  for (i = 0; i < n; ++i) {
+    fn(actual, expected);


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.

apapirovski · 2018-01-23T19:05:38Z

benchmark/assert/deepequal-object.js

    case 'deepStrictEqual':
      bench.start();
      for (i = 0; i < n; ++i) {
-        assert.deepStrictEqual(actual, expected);
+        fn(actual, expected);


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.

apapirovski · 2018-01-23T19:05:38Z

benchmark/buffers/buffer-base64-encode.js

-  let s = '';
-  let i;
+  var s = '';
+  var i;


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.

apapirovski · 2018-01-23T19:05:38Z

benchmark/buffers/buffer-bytelength.js

+  var strings = [ Buffer.alloc(len * 16, 'a') ];
+  var results = [ len * 16 ];
+
+  if (encoding !== 'buffer') {


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.

apapirovski · 2018-01-23T19:05:38Z

benchmark/buffers/buffer-read-float.js

  bench.start();
-  testFunction(buff);
-  bench.end(len / 1e6);
+  for (var i = 0; i !== millions * 1e6; i++) {


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?

apapirovski · 2018-01-23T19:05:38Z

benchmark/buffers/buffer-read-with-byteLength.js

  bench.start();
-  testFunction(buff);
-  bench.end(len / 1e6);
+  for (var i = 0; i !== millions * 1e6; i++) {


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

apapirovski · 2018-01-23T19:05:38Z

benchmark/path/basename-win32.js

@@ -28,7 +28,7 @@ function main({ n, pathext }) {

  bench.start();
  for (var i = 0; i < n; i++) {
-    posix.basename(pathext, ext);
+    win32.basename(pathext, ext);


Nice catch.

apapirovski · 2018-01-23T19:05:38Z

benchmark/tls/convertprotocols.js

  var m = {};
  // First call dominates results
  if (n > 1) {
    tls.convertNPNProtocols(['ABC', 'XYZ123', 'FOO'], m);
    m = {};
  }
  bench.start();
-  for (; i < n; i++) tls.convertNPNProtocols(['ABC', 'XYZ123', 'FOO'], m);
+  for (var i = 0; i < n; i++)
+    tls.convertNPNProtocols(['ABC', 'XYZ123', 'FOO'], m);


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

BridgeAR · 2018-01-24T13:02:10Z

@apapirovski PTAL

BridgeAR · 2018-01-24T13:39:40Z

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

apapirovski · 2018-01-24T14:20:10Z

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

BridgeAR · 2018-01-27T02:35:15Z

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.

BridgeAR · 2018-02-01T08:47:45Z

Rebased due to conflicts.

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

MylesBorins · 2018-03-20T15:54:10Z

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.

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

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

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

ryzokuken · 2018-06-06T05:17:25Z

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

MylesBorins · 2018-06-06T06:17:31Z

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

ryzokuken · 2018-06-06T09:18:08Z

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

MylesBorins · 2018-06-06T09:19:40Z

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

ryzokuken · 2018-06-06T09:20:18Z

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 · 2018-06-06T09:21:03Z

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

ryzokuken · 2018-06-06T09:39:17Z

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

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

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

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

nodejs-github-bot added assert async_hooks benchmark buffer crypto dgram domain v8 engine labels Jan 23, 2018

apapirovski reviewed Jan 23, 2018

View changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare Jan 26, 2018

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare Jan 26, 2018

BridgeAR force-pushed the improve-benchmarks branch from 982cf6d to 3f238eb Compare Jan 27, 2018

jasnell approved these changes Jan 29, 2018

View changes

BridgeAR added the author ready label Jan 30, 2018

BridgeAR added 10 commits Feb 1, 2018

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 Feb 1, 2018

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: fix platform in basename-win32

e673f94

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: fix variables not being set

13d28f5

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (assert) refactor

ab35472

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (buffer) refactor

0cd9273

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (crypto) refactor

158517d

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (url) refactor

c464c08

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (es) refactor

484b993

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (http(2)) refactor

187579b

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: (timers) refactor

59e0bbe

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

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018

benchmark: refactor

84db680

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: fix platform in basename-win32

16a3c7c

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: fix variables not being set

57d8ae4

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (assert) refactor

d4bc6c0

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (buffer) refactor

03e42df

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (crypto) refactor

e4e3490

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (url) refactor

ecb3fd5

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (es) refactor

cf8a4af

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (http(2)) refactor

8a97757

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: (timers) refactor

b186741

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

ryzokuken pushed a commit to ryzokuken/node that referenced this issue Jun 6, 2018

benchmark: refactor

ab4365e

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

ryzokuken mentioned this pull request Jun 6, 2018

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

Closed

4 tasks

BridgeAR deleted the improve-benchmarks branch Apr 1, 2019

nodejs / node Public

benchmarks: improve code base #18320

benchmarks: improve code base #18320

BridgeAR commented Jan 23, 2018

apapirovski left a comment

apapirovski Jan 23, 2018

BridgeAR Jan 24, 2018

apapirovski Jan 23, 2018

BridgeAR Jan 24, 2018

apapirovski Jan 23, 2018

apapirovski Jan 23, 2018

apapirovski Jan 23, 2018

BridgeAR Jan 24, 2018

apapirovski Jan 24, 2018

bmeurer Jan 25, 2018

BridgeAR Jan 27, 2018

apapirovski Jan 23, 2018

apapirovski Jan 23, 2018

apapirovski Jan 23, 2018

BridgeAR commented Jan 24, 2018

BridgeAR commented Jan 24, 2018

apapirovski commented Jan 24, 2018

BridgeAR commented Jan 27, 2018

BridgeAR commented Feb 1, 2018

MylesBorins commented Mar 20, 2018

ryzokuken commented Jun 6, 2018

MylesBorins commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

MylesBorins commented Jun 6, 2018 •

edited

ryzokuken commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

nodejs / node Public

benchmarks: improve code base #18320

benchmarks: improve code base #18320

Conversation

BridgeAR commented Jan 23, 2018

Checklist

Affected core subsystem(s)

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Jan 24, 2018

BridgeAR commented Jan 24, 2018

apapirovski commented Jan 24, 2018

BridgeAR commented Jan 27, 2018

BridgeAR commented Feb 1, 2018

MylesBorins commented Mar 20, 2018

ryzokuken commented Jun 6, 2018

MylesBorins commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

MylesBorins commented Jun 6, 2018 • edited

ryzokuken commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

ryzokuken commented Jun 6, 2018

MylesBorins commented Jun 6, 2018 •

edited