http2: use util._extend instead of Object.assign

[jvelezpo]

Since util._extend() still wins currently over Object.assign
it is better to use it in terms of performance here in http2

Benchmark before

Benchmark after

 http2/headers.js benchmarker='h2load' nheaders=0 n=1000                    0.13 %       ±1.81% ±2.39% ±3.07%
 http2/headers.js benchmarker='h2load' nheaders=1000 n=1000        ***      9.42 %       ±1.88% ±2.48% ±3.19%
 http2/headers.js benchmarker='h2load' nheaders=100 n=1000         ***      5.91 %       ±1.31% ±1.73% ±2.22%
 http2/headers.js benchmarker='h2load' nheaders=10 n=1000            *      1.55 %       ±1.23% ±1.62% ±2.08%

Be aware that when doing many comparisions the risk of a false-positive
result increases. In this case there are 4 comparisions, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
Notifying upstream projects of job completion
Finished: SUCCESS

Refs: #18707
Refs: #18442

Checklist

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

Affected core subsystem(s)

http2, test

[apapirovski]

These aren't statistically significant results, unfortunately. Most of the http2 benchmarks have a huge amount of fluctuation (the simple.js one in particular can easily fluctuate as much as 100% between two runs) and need to be run a minimum 100 times, ideally even more, to get any sort of significant data.

You can use node benchmark/compare.js --old ./node-master --new ./node --runs 100 http2 > results.csv & cat results.csv | Rscript benchmark/compare.R to get some meaningful data. (Where node-master would be a copy of the node build from master.) See our benchmarking guide: https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md

I'll trigger our benchmark CI job for this but my initial expectation is that this will only make a significant difference in the http2/headers benchmark with 1000 headers.

The fact that the ordering of the keys changes could be problematic. This also means that Node won't be able to benefit from any future improvements of Object.assign performance.

[apapirovski]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/125/

[jvelezpo]

thanks for triggering the CI @apapirovski
and for the feedback i will run the benchmark next time as you said 👍

Also I was about to open a new Issue to ask about the ordering or the util._extend, my question is if we change it (even if it is deprecated) to behave like Object.assign in terms of ordering the keys, would it be a breaking change? (i am thinking it would be breaking change just want to be sure).

[apapirovski]

Also I was about to open a new Issue to ask about the ordering or the util._extend, my question is if we change it (even if it is deprecated) to behave like Object.assign in terms of ordering the keys, would it be a breaking change? (i am thinking it would be breaking change just want to be sure).

I'm not a fan of making changes to deprecated code. And yes, it likely is a breaking change. As unlikely as it is, someone could rely on the exact order.

[jvelezpo]

In this case you only ran http2/headers.js
and here is the result

                                                            confidence improvement accuracy (*)   (**)  (***)
 http2/headers.js benchmarker='h2load' nheaders=0 n=1000                    0.13 %       ±1.81% ±2.39% ±3.07%
 http2/headers.js benchmarker='h2load' nheaders=1000 n=1000        ***      9.42 %       ±1.88% ±2.48% ±3.19%
 http2/headers.js benchmarker='h2load' nheaders=100 n=1000         ***      5.91 %       ±1.31% ±1.73% ±2.22%
 http2/headers.js benchmarker='h2load' nheaders=10 n=1000            *      1.55 %       ±1.23% ±1.62% ±2.08%

Be aware that when doing many comparisions the risk of a false-positive
result increases. In this case there are 4 comparisions, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
Notifying upstream projects of job completion
Finished: SUCCESS

One question, why didnt you run the other 3 benchmarks that there are for http2? (respond-with-fd, simple and write)

[apapirovski]

@jvelezpo It looks like we can't run those since h2load is not installed on that system.

Error: Requested benchmarker 'h2load' is  not installed
    at Object.exports.run (/home/iojs/build/workspace/benchmark-node-micro-benchmarks/benchmarking/experimental/benchmarks/community-benchmark/node/benchmark/_http-benchmarkers.js:203:14)
    at Benchmark.http (/home/iojs/build/workspace/benchmark-node-micro-benchmarks/benchmarking/experimental/benchmarks/community-benchmark/node/benchmark/common.js:119:21)
    at Http2Server.server.listen (/home/iojs/build/workspace/benchmark-node-micro-benchmarks/benchmarking/experimental/benchmarks/community-benchmark/node/benchmark/http2/respond-with-fd.js:28:13)
    at Object.onceWrapper (events.js:223:13)
    at Http2Server.emit (events.js:131:13)
    at emitListeningNT (net.js:1415:10)
    at process._tickCallback (internal/process/next_tick.js:114:19)

[jasnell]

Yeah, the CI does not yet have everything it needs for those. /cc @nodejs/build

I'll get an issue opened in the build repo for that.

For this particular PR, I'm not quite sold on this as I would prefer not to continue using the deprecated and non-idiomatic util._extend() despite it's improved performance in the microbenchmarks.

[BridgeAR]

@jasnell @apapirovski the function got deprecated to make sure it is not used outside of Node.js. I personally feel like we should use it until Object.assign is on par. Switching as soon as that happens is easy.

[apapirovski]

@BridgeAR Well, the semantics aren't strictly the same either as evidenced by the tests needing to change. I don't feel like the upside is worth the change here. I could be wrong...

[BridgeAR]

@apapirovski the tests rely on the order. It is a bit weird that the keys get inserted from behind in _extend. I would normally say, let us just switch to the regular order. It is used quite frequently throughout /lib though and that would would be a much bigger breakage.

We might do that nevertheless and just declare this as a semver-major?

[jvelezpo]

@BridgeAR and @apapirovski i did a test on my local changing the _extend function to run in the same keys order a Object.assign and after running the test, there was nothing affected

function _extend(target, source) {
  // Don't do anything if source isn't an object
  if (source === null || typeof source !== 'object') return target;

  const keys = Object.keys(source);
  const i = keys.length;
  let j = 0;
  while (j < i) {
    target[keys[j]] = source[keys[j]];
    j++;
  }
  return target;
}

Also we dont know outside of node as @apapirovski says As unlikely as it is, someone could rely on the exact order.

maybe this can be another PR 🤔

[BridgeAR]

We can also add another internal extend function that has the proper order.

[jvelezpo]

Another internal _ extend function with the proper order +1 on that

[jasnell]

Keep in mind that it is entirely possible (and likely) that the reverse order iteration is small part of the reason why this method may be faster.node

This is just a quick performance.timerify spot check with an object with 100 properties and not a proper benchmark, but reversing the order of the iteration definitely appears to have an impact.

Durations of multiple runs with forward iteration

0.227556
0.110131
0.076216
0.071111
0.049596
0.10612
0.064182
0.254542
0.097732
0.094814
0.067465
0.053607
0.065276
0.086063
0.153162
0.087521
0.07184
0.080592

Durations of multiple runs of util._extend

0.085333               
0.064183               
0.098097               
0.042302               
0.238496               
0.066735               
0.083145               
0.092992               
0.075487               
0.058348               
0.057983               
0.094815               
0.050689               
0.059807               
0.046314               
0.084604               
0.057983               
0.075487               
0.094086               
0.070746               
0.059807               
0.081686               

[BridgeAR]

@jasnell I just checked and all my variants also show a different performance profile.

@bmeurer I am a bit surprised that iterating down is faster than iterating up. Is there a specific reason for that?

Example code:

function _extend1(target, source) {
  if (source === null || typeof source !== 'object') return target;

  const keys = Object.keys(source);
  var i = keys.length;
  while (i--) {
    target[keys[i]] = source[keys[i]];
  }
  return target;
}

function _extend2(target, source) {
  if (source === null || typeof source !== 'object') return target;

  const keys = Object.keys(source);
  var i = 0;
  while (i < keys.length) {
    target[keys[i]] = source[keys[i++]];
  }
  return target;
}


function bench(fn) {
  console.time(`Runtime ${fn.name}`);
  for (var i = 0; i < 4e5; i++) {
    input[i % 100] = -input[i % 100];
    fn({}, input);
  }
  console.timeEnd(`Runtime ${fn.name}`);
}

const input = {};
for (var i = 0; i < 20; i++) {
  input['a' + (i % 2 * -1)] = 'foo' + i;
}

for (i = 0; i < 20; i++) {
  bench(_extend2);
  bench(_extend1);
}

[estrada9166]

I am a bit surprised that iterating down is faster than iterating up. Is there a specific reason for that?

@BridgeAR I think that the part of the answer, is that in this case you cache the length of the array, so each time it loops, it doesn't have to calc the length of it.... But also the reverse loop or iterating down in most of the cases is faster than the iterating up, here is a good benchmark to check it in jsperf

[apapirovski]

V8 is able to cache the length according to @bmeurer so I don't think that's the issue. Doing a quick test in repl reveals that to be the case.

As far as I can tell, iterating backwards and forwards has equivalent performance now. Which means that this must be related to some sort of property access caching or something, where retrieving the most recently stored values and going backwards is faster than the the other way around.

[benjamingr]

jsperf is a pretty awful tool IMO for measuring benchmark results. I'd like feedback from V8 on why Object.assign is performing worse in this case - and I'd also love to see a test with object spread to compare the results.

It would also really surprise me if these cases cause any measurable difference (just copying settings around). Moreover, I'm not even sure why we create a copy in some of these places like in https://github.com/nodejs/node/pull/18766/files#diff-696b2cc418addca5f3fe5020058f8b15R678 .

[apapirovski]

jsperf is a pretty awful tool IMO for measuring benchmark results. I'd like feedback from V8 on why Object.assign is performing worse in this case - and I'd also love to see a test with object spread to compare the results.

Check out the benchmark folder. @jasnell added a benchmark recently, it shows that both spread & Object.assign are significantly slower.

ping @bmeurer re: Object.assign performance. Can anything be done? It's surprising that it's like 3-4x slower than a for loop.

[jvelezpo]

It would also really surprise me if these cases cause any measurable difference (just copying settings around). Moreover, I'm not even sure why we create a copy in some of these places like in https://github.com/nodejs/node/pull/18766/files#diff-696b2cc418addca5f3fe5020058f8b15R678 .

Agree, there are some places where a copy does not make sense, i will make a change there in a bit.

[BridgeAR]

@apapirovski see #16081 (comment)

In the tracking bug there is a comment that someone recently got assigned to work on this :-)

[apapirovski]

@BridgeAR Ah, thanks. Hadn't seen that :)

[benjamingr]

@apapirovski first of all - it's nice that we have a benchmark for that (so props jasnell). I'm not sure I agree with it entirely though :P

If you look at https://github.com/nodejs/node/blob/master/benchmark/misc/util-extend-vs-object-assign.js#L28 it calls it with a proxy object, Object.assign has different set behavior from object spread and util._extend as far as I know so it's different behavior rather than just different spread.

If you meant another benchmark I'd love to see it.

On a tangent - is there any reason we don't use V8 natives syntax in benchmarks? The way we "force optimize" is flakey and V8 provides a function to %OptimizeFunctionOnNextCall - the type feedback we're giving it (process.env) depends on the running environment too which isn't too great.

[bmeurer]

Object.assign is non-trivial, but we have ideas. the simple for loop beats Object.assign when all ICs are monomorphic, but that is not always the case. I predict that it'll still take a while for Object.assign to reach peak performance level.

[jvelezpo]

@benjamingr did the change in the line you suggested and i get this error while testing.

It is there to create an empty object when settings is undefined, so i think it is better to leave there.

python tools/test.py -J --mode=release parallel/test-http2-getpackedsettings
=== release test-http2-getpackedsettings ===
Path: parallel/test-http2-getpackedsettings
internal/http2/core.js:679
                    settings.headerTableSize,
                             ^

TypeError: Cannot read property 'headerTableSize' of undefined
    at validateSettings (internal/http2/core.js:679:30)
    at Object.getPackedSettings (internal/http2/core.js:2680:24)
    at Object.<anonymous> (/Users/sebastian/Desktop/TEMP/node/test/parallel/test-http2-getpackedsettings.js:90:24)
    at Module._compile (module.js:666:30)
    at Object.Module._extensions..js (module.js:677:10)
    at Module.load (module.js:578:32)
    at tryModuleLoad (module.js:518:12)
    at Function.Module._load (module.js:510:3)
    at Function.Module.runMain (module.js:707:10)
    at startup (bootstrap_node.js:189:16)
Command: out/Release/node /Users/sebastian/Desktop/TEMP/node/test/parallel/test-http2-getpackedsettings.js
[00:00|% 100|+   0|-   1]: Done

[BridgeAR]

How shall we progress here?

[jasnell]

For now I think I'm -1 on this change.

[BridgeAR]

I am closing this due to the mentioned concerns.

@jvelezpo thanks a lot for your contribution anyway! It is much appreciated.