-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
n-api: add APIs for per-instance state management #28682
n-api: add APIs for per-instance state management #28682
Conversation
f8cff8c
to
edbb22a
Compare
Another set of numbers from after the last code change:
|
@gabrielschulhof Not sure if you saw nodejs/abi-stable-node#378 (comment), but could you explain why we pass the shared |
07603ad
to
b9ca7de
Compare
// makes use of it after the call into the module completes, but the module | ||
// may have deallocated **this**, and along with it the place where _env is | ||
// stored. | ||
napi_env env = _env; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@addaleax. I have now changed the implementation to be entirely local. The new version has the following performance implications on my laptop:
|
// Test that the instance data finalizer gets called. | ||
assert.strictEqual( | ||
(spawnSync(process.execPath, [__filename, 'child'], { | ||
stdio: [process.stdin, 'pipe', process.stderr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
['inherit', 'pipe', 'inherit']
? Or is there a reason you wrote it like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't know you could write 'inherit'
for individual streams. I'll change it.
static void DeleteAddonData(napi_env env, void* raw_data, void* hint) { | ||
AddonData* data = raw_data; | ||
if (data->print) { | ||
printf("deleting addon data"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add \n
.
napi_value buffer = NULL; | ||
if (establish_callback_ref(env, info)) { | ||
NAPI_CALL(env, napi_create_external_buffer(env, | ||
sizeof(TestBufferFinalizer), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is actually legal C, sizeof(SomeFunctionName)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is that even if some compilers accept it, not all will (or do the right thing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I'm changing it to sizeof(napi_callback)
.
b9ca7de
to
3d3bcff
Compare
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:
However, when running locally on my laptop, the change seems not to affect the numbers (which makes sense):
I don't get it 🤷 |
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. |
@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++)); do
echo '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:
|
3d3bcff
to
019a6c6
Compare
Well, FWIW a rebase removes the performance discrepancy 🤷 |
Should there also be a test that verifies the finalization is run when a worker terminates? |
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 rendering the `napi_env` local. Fixes: nodejs/abi-stable-node#378
019a6c6
to
b0ae730
Compare
@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. |
doc/api/n-api.md
Outdated
@@ -4860,6 +4936,7 @@ This API may only be called from the main thread. | |||
[ECMAScript Language Specification]: https://tc39.github.io/ecma262/ | |||
[Error Handling]: #n_api_error_handling | |||
[Native Abstractions for Node.js]: https://github.com/nodejs/nan | |||
[worker threads]: https://nodejs.org/api/worker_threads.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This needs to be placed as the last ref, after other lower-cased refs.
…he Node.js environment
246153b
to
53d2ed5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
53d2ed5
to
01a73d1
Compare
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 rendering the `napi_env` local to each add-on. Fixes: nodejs/abi-stable-node#378 PR-URL: nodejs#28682 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 rendering the `napi_env` local to each add-on. Fixes: nodejs/abi-stable-node#378 PR-URL: #28682 Backport-PR-URL: #30537 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - upgrade npm to 6.13.7 (Michael Perrotte) [#31558](#31558) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - upgrade npm to 6.13.7 (Michael Perrotte) [#31558](#31558) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
macOS package notarization and a change in builder configuration The macOS binaries for this release, and future 10.x releases, are now being compiled on macOS 10.15 (Catalina) with Xcode 11 to support package notarization, a requirement for installing .pkg files on macOS 10.15 and later. Previous builds of Node.js 10.x were compiled on macOS 10.10 (Yosemite) with a minimum deployment target of macOS 10.7 (Lion). As binaries are still being compiled to support a minimum of macOS 10.7 (Lion) we do not anticipate this having a negative impact on Node.js 10.x users with older versions of macOS. Notable changes: - buffer: add {read|write}Big\[U\]Int64{BE|LE} methods (garygsc) [#19691](#19691) - build: macOS package notarization (Rod Vagg) [#31459](#31459) - deps: - update npm to 6.14.3 (Myles Borins) [#32368](#32368) - upgrade openssl sources to 1.1.1e (Hassaan Pasha) [#32328](#32328) - upgrade to libuv 1.34.2 (cjihrig) [#31477](#31477) - n-api: - add napi\_get\_all\_property\_names (himself65) [#30006](#30006) - add APIs for per-instance state management (Gabriel Schulhof) [#28682](#28682) - define release 6 [#32058](#32058) - turn NAPI\_CALL\_INTO\_MODULE into a function (Anna Henningsen) [#26128](#26128) - tls: - expose keylog event on TLSSocket (Alba Mendez) [#27654](#27654) - support TLS min/max protocol defaults in CLI (Sam Roberts) [#27946](#27946) - url: handle quasi-WHATWG URLs in urlToOptions() (cjihrig) [#26226](#26226) PR-URL: #31984
Hi! I'm interested in using this feature in neon. What is required for this feature to move from experimental to stable? |
Adds
napi_set_instance_data()
andnapi_get_instance_data()
, whichallow native addons to store their data on and retrieve their data from
napi_env
.napi_set_instance_data()
accepts a finalizer which iscalled when the
node::Environment()
is destroyed.This entails splitting the
napi_env
into two portions. One portion isstored as a
napi_local_env
unique to each addon in such an addon'scallback wrappers and finalizer wrappers, and refers to the shared
portion, and the shared portion, the existing
napi_env
, which remainspretty 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 localportion.
Fixes: nodejs/abi-stable-node#378
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesHere is the performance impact relative to
git-base n-api-instance-data master
: