n-api: add APIs for per-instance state management

[gabrielschulhof]

Adds napi_set_instance_data() and napi_get_instance_data(), which
allow native addons to store their data on and retrieve their data from
napi_env. napi_set_instance_data() accepts a finalizer which is
called when the node::Environment() is destroyed.

This entails splitting the napi_env into two portions. One portion is
stored as a napi_local_env unique to each addon in such an addon's
callback wrappers and finalizer wrappers, and refers to the shared
portion, and the shared portion, the existing napi_env, which remains
pretty much unchanged, except that it now has a field which can be
updated to the local portion.

This field is updated with the local portion when the implementation is
poised to enter the addon. Thus, calls to napi_get_instance_data()
and napi_set_instance_data() will find and use the correct local
portion.

Fixes: nodejs/abi-stable-node#378

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

Here is the performance impact relative to git-base n-api-instance-data master:

 confidence improvement accuracy (*) (**) (***) napi/function_call n=1000000 type='napi' -0.78 % ±1.91% ±2.54% ±3.30% napi/function_call n=10000000 type='napi' -1.28 % ±1.55% ±2.06% ±2.68% napi/function_call n=50000000 type='napi' ** -2.88 % ±1.66% ±2.21% ±2.88% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 3 comparisons, you can thus expect the following amount of false-positive results: 0.15 false positives, when considering a 5% risk acceptance (*, **, ***), 0.03 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***) 

[gabrielschulhof]

Another set of numbers from after the last code change:

 confidence improvement accuracy (*) (**) (***) napi/function_call n=1000000 type='napi' -0.82 % ±1.70% ±2.27% ±2.96% napi/function_call n=10000000 type='napi' 0.30 % ±2.41% ±3.23% ±4.26% napi/function_call n=50000000 type='napi' *** -2.73 % ±1.33% ±1.78% ±2.34% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 3 comparisons, you can thus expect the following amount of false-positive results: 0.15 false positives, when considering a 5% risk acceptance (*, **, ***), 0.03 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***) 

[addaleax]

@gabrielschulhof Not sure if you saw nodejs/abi-stable-node#378 (comment), but could you explain why we pass the shared napi_env to the addons instead of the local ones? That would seem like a simpler solution, and I’m assuming that repeatedly setting/getting shared->local is the source of the performance impact here?

[gabrielschulhof]

By changing the expected signature of the lambda(s) being passed into CallIntoModule*() to receive a napi_env env we can avoid having to back up env like this, because the lambdas no longer access data from the instance.

[gabrielschulhof]

@addaleax. I have now changed the implementation to be entirely local. The new version has the following performance implications on my laptop:

 confidence improvement accuracy (*) (**) (***) napi/function_call n=1000000 type='napi' -2.48 % ±9.45% ±12.58% ±16.40% napi/function_call n=10000000 type='napi' -0.35 % ±5.92% ±7.88% ±10.25% napi/function_call n=50000000 type='napi' -1.15 % ±3.61% ±4.81% ±6.26% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 3 comparisons, you can thus expect the following amount of false-positive results: 0.15 false positives, when considering a 5% risk acceptance (*, **, ***), 0.03 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***) 

[bnoordhuis]

['inherit', 'pipe', 'inherit']? Or is there a reason you wrote it like this?

[gabrielschulhof]

Oh, I didn't know you could write 'inherit' for individual streams. I'll change it.

[bnoordhuis]

Might want to add \n.

[bnoordhuis]

I don't think this is actually legal C, sizeof(SomeFunctionName).

[gabrielschulhof]

Well, I did run make test shrug. I figure it'd grab the size of the function pointer. I can go with sizeof(napi_callback).

[bnoordhuis]

What I mean is that even if some compilers accept it, not all will (or do the right thing.)

[gabrielschulhof]

OK. I'm changing it to sizeof(napi_callback).

[gabrielschulhof]

OK, I have (what is to me) a mystery on my hands. The change

diff --git a/src/env.h b/src/env.h index 2bcb07a633..343876d073 100644 --- a/src/env.h+++ b/src/env.h@@ -149,7 +149,6 @@ constexpr size_t kFsStatsBufferLength = V(contextify_context_private_symbol, "node:contextify:context") \ V(contextify_global_private_symbol, "node:contextify:global") \ V(decorated_private_symbol, "node:decorated") \ - V(napi_env, "node:napi:env") \ V(napi_wrapper, "node:napi:wrapper") \ V(sab_lifetimepartner_symbol, "node:sharedArrayBufferLifetimePartner") \ 

should be part of this PR, because we now longer need that key. However, when I add this change to the PR, the numbers seem to drop precipitously when running on a server:

 confidence improvement accuracy (*) (**) (***) napi/function_call n=1000000 type='napi' 0.69 % ±5.99% ±8.06% ±10.69% napi/function_call n=10000000 type='napi' *** -3.92 % ±1.81% ±2.41% ±3.15% napi/function_call n=50000000 type='napi' *** -4.74 % ±2.28% ±3.05% ±4.00% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 3 comparisons, you can thus expect the following amount of false-positive results: 0.15 false positives, when considering a 5% risk acceptance (*, **, ***), 0.03 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***) 

However, when running locally on my laptop, the change seems not to affect the numbers (which makes sense):

 confidence improvement accuracy (*) (**) (***) napi/function_call n=1000000 type='napi' 0.33 % ±3.67% ±4.89% ±6.36% napi/function_call n=10000000 type='napi' -0.37 % ±2.08% ±2.77% ±3.61% napi/function_call n=50000000 type='napi' -0.91 % ±1.25% ±1.67% ±2.18% Be aware that when doing many comparisons the risk of a false-positive result increases. In this case there are 3 comparisons, you can thus expect the following amount of false-positive results: 0.15 false positives, when considering a 5% risk acceptance (*, **, ***), 0.03 false positives, when considering a 1% risk acceptance (**, ***), 0.00 false positives, when considering a 0.1% risk acceptance (***) 

I don't get it 🤷

[bnoordhuis]

My first hunch would be L1 cache ping-ponging. You should be able to check with perf(1) when you run one of the benchmarks in isolation.

[gabrielschulhof]

@bnoordhuis I'm pretty green at this, but I tried counting cache misses, and there doesn't seem to be a big difference. I did ten runs each with the two versions (with and without the above-mentioned change) using the following command line:

for((Nix=0; Nix <10; Nix++));doecho'with env' perf stat -e cache-misses \ ./node \ --perf-basic-prof \ benchmark/napi/function_call/index.js \ type=napi \ n=50000000 echo'no env' perf stat -e cache-misses \ ../node.no-env \ --perf-basic-prof \ benchmark/napi/function_call/index.js \ type=napi \ n=50000000 done

The resulting cache misses were:

	with key removal	without key removal
average	331928.888888889	323810.111111111
stdev	79864.8042998987	42263.3076836292

[gabrielschulhof]

Well, FWIW a rebase removes the performance discrepancy 🤷

[mhdawson]

Should there also be a test that verifies the finalization is run when a worker terminates?

[gabrielschulhof]

@mhdawson I have now added a paragraph about the Agent and its Node.js equivalent. I have also re-written the test to run both on the main thread and on a worker thread.

[vsemozhetbyt]

Nit: This needs to be placed as the last ref, after other lower-cased refs.

[mhdawson]

LGTM

[kjvalencik]

Hi! I'm interested in using this feature in neon.

What is required for this feature to move from experimental to stable?