test : millions and thounsand changed to n in benchmark #18917

juggernaut451 · 2018-02-21T20:42:50Z

This is refactoring of the benchmark.
It contained millions or thousands which is changed to n

Fixes : #18778

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, benchmark

BridgeAR · 2018-02-22T01:04:46Z

benchmark/buffers/buffer-compare-instance-method.js

-function main({ millions, size, args }) {
-  const iter = millions * 1e6;
+function main({ n, size, args }) {
+  const iter = n * 1e6;


Hm, it seems like the description in #18778 was not clear enough: it was not about just replacing the variable name. Instead of having millions = 1 it should be n = 1e6, not n = 1. That applies to all of these changes.

This also has influence on the for loops and the benchmark end call. These should be changed accordingly.

If there is a iter variable as here, it is likely obsolete and it could be replaced with n instead.

working on it.

BridgeAR · 2018-02-22T01:09:22Z

@juggernaut451 since you opened multiple pull requests so far: would you be so kind and please have another look at the commit guidelines?

juggernaut451 · 2018-02-22T06:15:16Z

@BridgeAR changed the commit message. Thank you for pointing out.

BridgeAR · 2018-02-22T13:54:11Z

@juggernaut451 So far nothing is changed. I guess you did not push your commit. If you want to change the commit message, you have to change the commit history by force pushing. Please use git push --force-with-lease.

juggernaut451 · 2018-02-22T16:38:20Z

@BridgeAR sorry forgot to push the message. Can you please check now?

starkwang · 2018-02-23T07:54:27Z

benchmark/es/defaultparams-bench.js

@@ -5,7 +5,7 @@ const assert = require('assert');

 const bench = common.createBenchmark(main, {
  method: ['withoutdefaults', 'withdefaults'],
-  millions: [100]
+  n: [100e6]


I'd like to use 1e8 for consistency. (ditto throughout)

@starkwang so everywhere I should replace the value of n with 1e8?

I am not sure what the question stands for. Is there something unclear with the suggestion from @starkwang ?

@juggernaut451 100e6 means 100 * 1e6, 1e6 means 10^6. Thus 100e6 can be written as 1e8.

changes made. Thanks

AndreasMadsen · 2018-02-26T08:53:55Z

test/parallel/test-benchmark-module.js

@@ -5,7 +5,7 @@ require('../common');
 const runBenchmark = require('../common/benchmark');

 runBenchmark('module', [
-  'thousands=.001',
+  'n=.001',


changes made. Thanks

BridgeAR

Besides the comments there are a couple of tests that still need to be changed as well. The millions and thousands entries should be replaced in that case.

BridgeAR · 2018-02-24T15:12:33Z

benchmark/buffers/buffer-compare-instance-method.js

        b0.compare(b1, 0);
      }
-      bench.end(iter / 1e6);
+      bench.end(n / 1e6);


bench.end should never say n / x. Instead, it should always only be n. That applies to all benchmarks and is missing in many files here.

Mostly correct. There are cases where we see how many requests a server can take over 10 seconds and then we run bench.end(numberOfRequests) but those are special cases.

BridgeAR · 2018-02-24T15:15:04Z

benchmark/es/defaultparams-bench.js

@@ -5,7 +5,7 @@ const assert = require('assert');

 const bench = common.createBenchmark(main, {
  method: ['withoutdefaults', 'withdefaults'],
-  millions: [100]
+  n: [100e6]


I am not sure what the question stands for. Is there something unclear with the suggestion from @starkwang ?

BridgeAR · 2018-02-27T08:59:05Z

benchmark/timers/immediate.js

@@ -2,113 +2,113 @@
 const common = require('../common.js');

 const bench = common.createBenchmark(main, {
-  thousands: [5000],
+  n: [5000e3],


The comment from @starkwang applies here as well and in more places.

BridgeAR · 2018-02-27T09:02:35Z

benchmark/timers/immediate.js

-function depth(N) {
-  var n = 0;
+function depth(n) {
+  var i = 0;


This is a independent change and is not necessary. Please change it to the way it was before to reduce churn. This applies to all these functions in this test.

BridgeAR · 2018-02-27T09:11:21Z

benchmark/buffers/dataview-set.js

 }

-function benchInt(dv, fn, len, le) {
+function benchInt(dv, fn, n, le) {


Renaming this variable is not necessary. I would rather change it to the way as it was before to reduce churn. That applies to all functions in this test besides main.

In this const len = millions * 1e6 was defined. There is no need of len as it's value is n. So that's why I replaced len with n. If I don't replace then I have to initialize the len

So what I meant is that only the main function is impacted by that change. You can keep the len argument here and only change len / 1e6 to len. That way there is less churn.

that sounds good. Thanks :)

changes made.

BridgeAR · 2018-02-27T09:14:59Z

benchmark/process/next-tick-breadth-args.js

-      bench.end(n / 1e6);
+    j++;
+    if (j === n)
+      bench.end(i / 1e6);


Please do not use i but n here and only n and no division as mentioned in another comment. That applies to all cases in this test.

changes made :)

BridgeAR · 2018-02-27T09:15:20Z

benchmark/process/next-tick-breadth-args.js

-  const N = millions * 1e6;
-  var n = 0;
+function main({ n }) {
+


Nit: please remove the line break here.

changes made :)

BridgeAR · 2018-02-27T09:16:01Z

benchmark/process/next-tick-depth-args.js

-function main({ millions }) {
-  var n = millions * 1e6;
+function main({ n }) {
+



Please do not add double line breaks. This is a unnecessary change.

changes made :)

BridgeAR

This looks quite nice now.

BridgeAR · 2018-02-28T17:02:57Z

benchmark/buffers/dataview-set.js

 }

-function benchInt(dv, fn, len, le) {
+function benchInt(dv, fn, n, le) {


So what I meant is that only the main function is impacted by that change. You can keep the len argument here and only change len / 1e6 to len. That way there is less churn.

AndreasMadsen

LGTM. Thanks, this is something that has annoyed me for a long time and confused new users, thus I'm very happy to see this is now fixed :)

Well done!

juggernaut451 · 2018-03-01T19:57:52Z

Thanks, @AndreasMadsen

apapirovski

LGTM but needs to be rebased and there's a small nit below.

apapirovski · 2018-03-04T14:24:55Z

benchmark/buffers/dataview-set.js

@@ -39,9 +39,9 @@ const mod = {
  setUint32: UINT32
 };

-function main({ millions, type }) {
+function main({ n, type }) {
+  const len = n;


This could just use n throughout the main function rather than creating an extra variable.

It is done to reduce the churn as requested by @BridgeAR

No, I actually explicitly excluded the main function in my comment ;-)

making the change on the main function too

changes made

starkwang · 2018-03-08T04:54:40Z

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

juggernaut451 · 2018-03-14T06:43:47Z

Resolved :)

starkwang · 2018-03-15T01:49:10Z

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

juggernaut451 · 2018-03-15T09:42:21Z

can someone help me in understanding what exactly is failing?

gibfahn · 2018-03-15T13:56:30Z

@juggernaut451 if you click on the CI link and scroll down to the bottom you'll see a link to node-test-commit:

If you click on that link you'll see that it failed, so you need to go to the Console Output on the right:

If you click that you'll see this:

+ git rebase --committer-date-is-author-date cd7d7b15c1eeccfe2facdf9a671034d93b6bf467
First, rewinding head to replay your work on top of it...
Applying: test : millions and thounsand changed to n in benchmark
Using index info to reconstruct a base tree...
M	benchmark/buffers/buffer-read-float.js
M	benchmark/buffers/buffer-read-with-byteLength.js
M	benchmark/buffers/buffer-read.js
M	benchmark/buffers/buffer-write.js
Falling back to patching base and 3-way merge...
Auto-merging benchmark/buffers/buffer-write.js
CONFLICT (content): Merge conflict in benchmark/buffers/buffer-write.js
Auto-merging benchmark/buffers/buffer-read.js
CONFLICT (content): Merge conflict in benchmark/buffers/buffer-read.js
Auto-merging benchmark/buffers/buffer-read-with-byteLength.js
CONFLICT (content): Merge conflict in benchmark/buffers/buffer-read-with-byteLength.js
Auto-merging benchmark/buffers/buffer-read-float.js
CONFLICT (content): Merge conflict in benchmark/buffers/buffer-read-float.js
error: Failed to merge in the changes.
Patch failed at 0001 test : millions and thounsand changed to n in benchmark
The copy of the patch that failed is found in: .git/rebase-apply/patch

I know that this PR isn't showing that there are conflicts, but the CI does the rebase in a different way.

I'm pretty sure this commit is the issue:

Basically what you need to do is to squash all your commits into one commit, and then rebase that on top of the upstream master branch.

juggernaut451 · 2018-03-17T18:50:26Z

Thank you @gibfahn

juggernaut451 · 2018-03-28T08:52:17Z

@starkwang can you please re run the CI

richardlau · 2018-03-28T09:11:00Z

CI is currently locked down for the security release: #19642

richardlau

"thousands" is misspelled in the commit message.

ChALkeR · 2018-03-28T10:04:07Z

Perhaps var should be changed to let in the touched lines?
That works down to 6.x and I don't think this is going to be backported to 4.x.

BridgeAR · 2018-04-09T10:42:02Z

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

@ChALkeR there are only very few lines that touch var in this PR. So I believe we should go ahead and land this instead of delaying it further for that.

BridgeAR · 2018-04-09T22:23:01Z

Landed in b80da63 🎉

PR-URL: nodejs#18917 Fixes: nodejs#18778 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

PR-URL: #18917 Fixes: #18778 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

BridgeAR reviewed Feb 22, 2018

View reviewed changes

juggernaut451 force-pushed the refactorBenchmark branch from 662fbb3 to bc65c16 Compare February 22, 2018 16:33

juggernaut451 changed the title ~~Fixes : https://github.com/nodejs/node/issues/18778~~ test : millions and thounsand changed to n in benchmark Feb 22, 2018

starkwang reviewed Feb 23, 2018

View reviewed changes

AndreasMadsen reviewed Feb 26, 2018

View reviewed changes

jasnell approved these changes Feb 26, 2018

View reviewed changes

starkwang approved these changes Feb 27, 2018

View reviewed changes

BridgeAR requested changes Feb 27, 2018

View reviewed changes

BridgeAR reviewed Feb 27, 2018

View reviewed changes

BridgeAR approved these changes Feb 28, 2018

View reviewed changes

AndreasMadsen approved these changes Mar 1, 2018

View reviewed changes

apapirovski approved these changes Mar 4, 2018

View reviewed changes

gibfahn approved these changes Mar 5, 2018

View reviewed changes

juggernaut451 force-pushed the refactorBenchmark branch from 1131890 to 3c15b97 Compare March 8, 2018 09:03

juggernaut451 force-pushed the refactorBenchmark branch from 3c15b97 to faece08 Compare March 17, 2018 15:20

test: millions and thounsand changed to n in benchmark

5a5c6a0

juggernaut451 force-pushed the refactorBenchmark branch from faece08 to 5a5c6a0 Compare March 17, 2018 17:51

juggernaut451 added 2 commits March 17, 2018 23:26

Undo changes while rebasing

4cd8171

Remove duplicacy

976067e

richardlau reviewed Mar 28, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018

jasnell approved these changes Apr 9, 2018

View reviewed changes

BridgeAR closed this Apr 9, 2018

test : millions and thounsand changed to n in benchmark #18917

test : millions and thounsand changed to n in benchmark #18917

Conversation

juggernaut451 commented Feb 21, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Feb 22, 2018

juggernaut451 commented Feb 22, 2018

BridgeAR commented Feb 22, 2018

juggernaut451 commented Feb 22, 2018

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

BridgeAR Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juggernaut451 Mar 1, 2018 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment • edited Loading

Choose a reason for hiding this comment

juggernaut451 commented Mar 1, 2018

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

starkwang commented Mar 8, 2018

juggernaut451 commented Mar 14, 2018

starkwang commented Mar 15, 2018

juggernaut451 commented Mar 15, 2018

gibfahn commented Mar 15, 2018

juggernaut451 commented Mar 17, 2018

juggernaut451 commented Mar 28, 2018

richardlau commented Mar 28, 2018

richardlau left a comment

Choose a reason for hiding this comment

ChALkeR commented Mar 28, 2018 • edited Loading

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 9, 2018

juggernaut451 commented Feb 21, 2018 •

edited

Loading

BridgeAR Feb 27, 2018 •

edited

Loading

BridgeAR Feb 27, 2018 •

edited

Loading

juggernaut451 Mar 1, 2018 •

edited

Loading

AndreasMadsen left a comment •

edited

Loading

ChALkeR commented Mar 28, 2018 •

edited

Loading