module: NativeModule._cache turned to a Map

[danyshaanan]

Checklist

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

Description of change

Changed NativeModule._cache in lib/internal/bootstrap_node.js to be a Map, in accordance to nodejs/code-and-learn#56 (comment)

[mscdex]

Would be interesting to see the performance impact of this first. /cc @nodejs/benchmarking

[danyshaanan]

@mscdex How can I check that? Are there any performance tests I can run?

[mscdex]

I don't think there is a native module loading benchmark in this repo at least (so far). I am not sure if there are any elsewhere or not. The benchmarking team might know.

[eljefedelrodeodeljefe]

LGTM, since I believe it is always outside of hot paths. The performance regression shouldn't be be massive really.

[danyshaanan]

$ #in master (a89c23f8):
$ make -j8
$ ./node benchmark/module/module-loader.js
module/module-loader.js fullPath="true" thousands=50: 1.5335868438348847
module/module-loader.js fullPath="false" thousands=50: 1.435314624271457
$ git checkout NativeModule-cache-map 
$ make -j8
$ ./node benchmark/module/module-loader.js
module/module-loader.js fullPath="true" thousands=50: 1.5309753684627516
module/module-loader.js fullPath="false" thousands=50: 1.3562813178146016

And if I'd want to run compare.js, what would X be in:

./node benchmark/compare.js --old out/Release/node-master --new ./node X

?

[mscdex]

@eljefedelrodeodeljefe IMHO it's a hot path since NativeModule.require and others are what are used when resolving/loading built-in modules (even after they are cached the first time -- since there is still the call to nativeModuleCacheMap.get() every time). It's not just a startup cost.

@danyshaanan That benchmark mostly exercises third party module loading, which goes through a different mechanism than the built-in ("native") modules.

[cjihrig]

I agree with @mscdex

[danyshaanan]

@mscdex Does that mean that that benchmark does not reflect the real use and effect of the code change? If so, how can this be properly checked?

[mscdex]

@danyshaanan Correct. If the benchmarking team doesn't already have one, a new benchmark would have to be added to test the native module loading mechanism. That would be kind of tricky to implement since I believe some modules are loaded during startup, so startup time would also have to be measured (I think there is already a benchmark for startup time).

[addaleax]

I’m not sure the startup time is noticeably affected by this (I also think that’s already benchmarked?), but it would probably be nice to see benchmarks for re-requiring builtin modules over and over again.

[Fishrock123]

@nodejs/benchmarking I know you run require cache benchmarks for https://benchmarking.nodejs.org/, could you run those on this?

[addaleax]

ping @nodejs/benchmarking again because that doesn’t seem to work from review comments?

[vkurchatkin]

But what's the point of this change, really? Is it supposed to be faster?

[addaleax]

Is it supposed to be faster?

afaik that was basically the outcome of the Object.create(null) discussions for the HTTP headers object or the querystring one.

[vkurchatkin]

Object.create(null) discussions for the HTTP headers object or the querystring one.

Well, the same logic is not applicable here, because we don't use untrusted strings as keys

[addaleax]

Well, the same logic is not applicable here, because we don't use untrusted strings as keys

I am not sure how that relates…? I mean, yes, the original reasons for the querystring/http discussions were untrusted keys, but the microbenchmarking result that Maps are faster for these kind of things than objects (prototypeless or not) that came out of those discussions is what would matter here.

(See #7581 for what that speed information is based on. I realize it would probably be good to benchmark this separately.)

[mscdex]

I'm not so sure about Map performance, as I saw conflicting results much later after the Map benchmarks were created, in something else I was benchmarking (can't recall offhand now what that was).

[jasnell]

@danyshaanan ... I cannot remember if we spoke at the code and learn about running the benchmark comparisons. It would be great to get some comparisons on the performance impact of this change. If you need some help collecting those, definitely let us know. As @mscdex points out, it's unclear if using Map yields material performance benefits as the results have been somewhat inconclusive so far.

As for the rest, I'm +1 on this change.

[danyshaanan]

@jasnell - We have, a bit. I ran what I could (#8576 (comment)). From Brian's comment (#8576 (comment)) I understand that a new benchmark is required, so if benchmark/README.md is up to date, I could look into it and give it a shot when I'll be back home... I guess that the correct thing would be to open an issue and move the benchmark-related discussion there.

[gareth-ellis]

I've had a go at running the require benchmarks locally.

The require test only requires a fairly basic .js file - not with native code. I might see if I can write a basic native module and add that as one of these require tests too.

In the mean time, here is the results of require tests as run on benchmarking.nodejs.org.

The results seem to be as below. The scores are in operations per second, so higher is better:

We're wanting to compare the pr-require.new with master-require.new and pr-require.cached with master-require.cached

We can see that we get a lower average ops/second for this pr, compared to the base build.

Caveats:

This data was not collected on the benchmarking machine, so can't be compared to the results displayed on benchmarking.nodejs.org

Looking at the data on benchmarking.nodejs.org, we can see there's quite a bit of variability in this benchmark already.

Early comments in this PR suggest that this may not even be testing what this PR changes, in which case either the differences we see above are due to variability, or some unexpected side effect.

I'll run some more measures (the above data was a set of 10, interleaving pr, master, pr etc.

[addaleax]

The require test only requires a fairly basic .js file - not with native code. I might see if I can write a basic native module and add that as one of these require tests too.

Just to be clear, in this context NativeModule refers to Node’s builtin JS modules like fs, util, http etc., not native C++ addons.

[ThePrimeagen]

@gareth-ellis with the results from the benchmarks being so close I would recommend running it quite a few times, say 30, on a benchmark machine, something with as little noise as possible and get the t-value.

That close of a micro-benchmark worries me :)

Second, I don't know how much time has been spent optimizing Map's underlying performance, but this is likely to be something we should keep our eye on over time as/if Maps performance is improved.

[jasnell]

Closing in favor of #10789 which touches on the same bits.