node-api: add napi_create_external_sharedarraybuffer#62623
node-api: add napi_create_external_sharedarraybuffer#62623bnoordhuis wants to merge 9 commits intonodejs:mainfrom
Conversation
Creates a SharedArrayBuffer from externally managed memory. Fixes: nodejs#62259
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62623 +/- ##
==========================================
+ Coverage 89.77% 89.80% +0.03%
==========================================
Files 697 699 +2
Lines 215749 216254 +505
Branches 41304 41350 +46
==========================================
+ Hits 193681 194204 +523
+ Misses 14161 14139 -22
- Partials 7907 7911 +4
🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
request-ci
ronag
added
the
author ready
legendecas
removed
the
author ready
| napi_env env, | ||
| void* external_data, | ||
| size_t byte_length, | ||
| void (*finalize_cb)(void* external_data, void* finalize_hint), |
There was a problem hiding this comment.
We typically do not use the inline function pointer types.
Can we use the node_api_basic_finalize here?
I guess it should OK to document there that env is null there.
There was a problem hiding this comment.
I did that intentionally, of course. Passing nullptr is just setting up users for segfaults.
There was a problem hiding this comment.
I think the semantic of this finalizer is different from node_api_basic_finalize. node_api_basic_finalize is still invoked on the JS thread. But this finalizer could be invoked on non-main-JS thread. I think it is worth a dedicated type.
There was a problem hiding this comment.
Then it would be better to define a new type alias for it. It is better to follow the established patterns.
src/js_native_api_v8.cc
Outdated
| if (finalize_cb != nullptr) { | ||
| deleter_data = new FinalizerData{finalize_cb, finalize_hint}; | ||
| } | ||
| auto unique_backing_store = v8::SharedArrayBuffer::NewBackingStore( |
There was a problem hiding this comment.
It is better to avoid auto for readability when possible.
IMHO, we should only use it for lambdas and iterators.
In other trivial cases like here using the real types can help readability and see the issues sooner like with the missing std::move below.
There was a problem hiding this comment.
The counterargument is that repeating oneself at the left- and right-hand side of an assignment often results in unwieldy line noise. auto is also used quite a bit elsewhere in this file, and not just for lambdas.
For the record: I'm pretty much the founder of Node's C++ house style and I've always been a-okay with judicious use of auto.
There was a problem hiding this comment.
I think it is okay to use auto when the type is clear: https://github.com/nodejs/node/blob/main/doc/contributing/cpp-style-guide.md#using-auto
I would find this auto good because the right hand side is clear that this is a v8 backing store.
|
Review feedback incorporated, PTAL. |
| // Caveat emptor: it's indeterminate when the SharedArrayBuffer's backing | ||
| // store is reclaimed; at least some of the time it happens even before | ||
| // calling gc(). | ||
| let sab = binding.newExternalSharedArrayBuffer(); |
There was a problem hiding this comment.
Minor suggestion:
| let sab = binding.newExternalSharedArrayBuffer(); | |
| let sab = binding.newExternalSharedArrayBuffer(); | |
| assert(util.types.isSharedArrayBuffer(sab)); |
| "defines": [ | ||
| 'NAPI_VERSION=10' | ||
| "NAPI_EXPERIMENTAL", | ||
| "NAPI_VERSION=10", |
There was a problem hiding this comment.
The NAPI_EXPERIMENTAL overrides NAPI_VERSION=10. It is better to avoid confusion and remove the NAPI_VERSION=10.
github-actions
bot
removed
the
request-ci
7 participants
Creates a SharedArrayBuffer from externally managed memory.
Fixes: #62259