Node: Reduce benchmark test run time

[BridgeAR]

Right now our benchmarks often contain millions or thousands as iteration step. This should be changed to n (in case of a conflict e.g. iterations) where the concrete number is given.

This allows our tests to significantly run faster, since the minimum iteration will not be a million times but a single time. The corresponding benchmark tests should be changed as well therefore.

Another benefit of it is that it is easier to read how fast the operation really was.

It would be great if a PR would change all entries at once or all entries of a subsystem to prevent conflicting changes. If someone starts working on this, please leave a note when you do so and on what you work so others know.

As soon as the benchmarks and the tests get fixed there would be a follow up task to check how long each individual sequential benchmark tests takes that was changed and if it would be possible to move them from sequential to parallel.

I will gladly assist as a mentor in case help is needed.

Update: I "removed the parts that do not apply anymore. The rest is still up to date and open.

[apapirovski]

This allows our tests to significantly run faster, since the minimum iteration will not be a million times but a single time. The corresponding benchmark tests should be changed as well therefore.

I don't believe this is correct. We set thousands to 0.001 to counteract that problem.

[BridgeAR]

@apapirovski seems like we missed to add the argument for the buffers test. That does not contain the millions entry even though it should. I stumbled across it when looking at the coverage. After seeing that some operations where executed over a million times, I expected it to be the test.

[apapirovski]

@BridgeAR good eye. Should be an easy change if someone wants to make a very simple but impactful first commit.

[juggernaut451]

i would like to take this up @BridgeAR @apapirovski

[juggernaut451]

@BridgeAR can you please tell exactly which file to work on?

[BridgeAR]

So the test file is /test/sequential/test-benchmark-buffer.js. So you can either add a millions = 1/1e6 entry. Or you work on the benchmarks and remove the millions entries there and switch to n instead. Just look for all millions and thousands entries in the /benchmark folder.

[juggernaut451]

@BridgeAR can i put millions = 0.000001 rather than 1/1e6 as Benchmark in common/benchmark.js set 1/1e6 as NaN

[BridgeAR]

@juggernaut451 yes, for me it was only a way of writing the number that should be passed in.

[AndreasMadsen]

I would definitely prefer the consistency only using n.

[BridgeAR]

I would also prefer to consistently use n. So this is definitely still up as good first/second contribution.

[juggernaut451]

@BridgeAR working on it..

[juggernaut451]

@BridgeAR in some cases n is already defined. Which may give arise to conflict when renaming millions -> n. For example in benchmark/es/spread-assign.js.

[AndreasMadsen]

In that case n is the number you want, so that shouldn't cause any problems.

const bench = common.createBenchmark(main, {
  method: ['spread', 'assign', '_extend'],
  count: [5, 10, 20],
- millions: [1]
+ n: [1e6]
});

- function main({ millions, context, count, rest, method }) {
+ function main({ n, context, count, rest, method }) {
-   const n = millions * 1e6;

[juggernaut451]

thank you @AndreasMadsen :)

[juggernaut451]

In benchmark/process/next-tick-depth-args.js there is already variable n that is being manipulate inside the file and bench.end is passing millions. Will there be conflict if i change millions -> n ?

[BridgeAR]

@juggernaut451 please always check how the variables are used. In this particular case it is that n is what we want to have. So you just have to replace millions with n and you are done.

diff --git a/benchmark/process/next-tick-depth-args.js b/benchmark/process/next-tick-depth-args.js
index 1c1b95bdc8..52c6f4f431 100644
--- a/benchmark/process/next-tick-depth-args.js
+++ b/benchmark/process/next-tick-depth-args.js
@@ -2,13 +2,12 @@

 const common = require('../common.js');
 const bench = common.createBenchmark(main, {
-  millions: [12]
+  n: [12e6]
 });

 process.maxTickDepth = Infinity;

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

   function cb4(arg1, arg2, arg3, arg4) {
     if (--n) {
@@ -21,7 +20,7 @@ function main({ millions }) {
       else
         process.nextTick(cb1, 0);
     } else
-      bench.end(millions);
+      bench.end(n);
   }
   function cb3(arg1, arg2, arg3) {
     if (--n) {
@@ -34,7 +33,7 @@ function main({ millions }) {
       else
         process.nextTick(cb1, 0);
     } else
-      bench.end(millions);
+      bench.end(n);
   }
   function cb2(arg1, arg2) {
     if (--n) {
@@ -47,7 +46,7 @@ function main({ millions }) {
       else
         process.nextTick(cb1, 0);
     } else
-      bench.end(millions);
+      bench.end(n);
   }
   function cb1(arg1) {
     if (--n) {
@@ -60,7 +59,7 @@ function main({ millions }) {
       else
         process.nextTick(cb1, 0);
     } else
-      bench.end(millions);
+      bench.end(n);
   }
   bench.start();
   process.nextTick(cb1, true);

[juggernaut451]

ok thanks was just confirming so that things don't mess up

[juggernaut451]

@BridgeAR benchmark/process/next-tick-breadth-args.js file already contain n and millions value is being allocated to N which is giving conflict as n is already defined

[juggernaut451]

ignore the last comment. Figured out the solution

[juggernaut451]

I have raised the pull request. Please review it