N-API SharedArrayBuffer api

[orange4glace]

Implementing :
napi_create_external_sharedarraybuffer
napi_get_sharedarraybuffer_info
napi_is_sharedarraybuffer

Since V8 api supports external sharedarraybuffer creation, it would be nice if n-api implements this.

[orange4glace]

Build failed

[aqrln]

Hi! Did you accidentally close you PR (presumably clicking on "Close and comment" instead of "Comment" button) or this was intentional?

[orange4glace]

I saw that the build was cancelled by CI. I thought there may be a compile error or something(I didnt test it my own before making a PR). So I closed the PR intentionally.

[addaleax]

@orange4glace It’s totally fine if a PR doesn’t pass CI at first – you can always create new changes and push to your branch to change them.

In this case, could you make sure that when you run make -j4 test locally, it passes?

[orange4glace]

Oh, I see. I am almost new to git so my apology. I am trying to build it with my Windows 10 but it keeps fail, even with vanila version of it. :| Maybe I'll have to try on linux so I can find out whether it works. Thanks!

[addaleax]

@orange4glace No problem – let us know if we can help in some way!

[gabrielschulhof]

@orange4glace napi_create_typedarray() should probably also be modified to work with SharedArrayBuffer.

[devsnek]

please remove this one as it isn't something that we can expect other engines to be able to implement

[addaleax]

@devsnek Just for context, we also have napi_create_external_arraybuffer

[devsnek]

@addaleax then let's not make the same mistake twice?

[orange4glace]

So the other engines do not support external arraybuffer and n-api has already some mis-design related to external-arraybuffer api things?

[mhdawson]

@digitalinfinity can you comment on Chakra support for napi_create_external_arraybuffer (which I think it should since we included in the API) and external shared array buffers?

It is an important question as to whether most engines support external shared array buffers ...

.

[mhdawson]

If this does move forward the new APIs need to start as experimental and be in the section guarded by #ifdef NAPI_EXPERIMENTAL

[mhdawson]

@orange4glace for reference in case you have not seen it yet. This covers some of the principles we want to follow for new APIs. https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md

I'll also add thank you for help with N-API.

And one final question, is there a use case where you need to use the external shared buffers that can't be covered with the existing API surface in another way?

[orange4glace]

@mhdawson Thanks for the link! I will try to follow those rules for future PRs :]

I am currently working on kind of video editor based on Electron. The N-API native module will be loaded on Browser process. So there is mainly three threads involved.

1. Browser main thread which also contains native module

2. A decoder thread, running ffmpeg decoding things.

3. Since I want to prevent UI blocking when such a heavy rendering works are going on(Webgl), I decided to use offscreen canvas rendering which is running on separate web worker.

So the second and third thread should share the buffer memory to prevent copying huge size of memory thus improves a performance. At first I came up with ArrayBuffer transfer via postMessage, but some reason the browser crashes when external ArrayBuffer transfer occurs. So rather than finding why, I focused on this SharedArrayBuffer feature.

[addaleax]

At first I came up with ArrayBuffer transfer via postMessage, but some reason the browser crashes when external ArrayBuffer transfer occurs.

I can only guess, but I think there might be a chance you’re running into the same problem that we have with externalized SharedArrayBuffers? Maybe Chromium does let you share them with WebWorkers, but you may experience similar issues with unclear memory ownership…

If this does move forward the new APIs need to start as experimental and be in the section guarded by #ifdef NAPI_EXPERIMENTAL

I’d disagree. This API fills in a gap from the ES spec and adds parity to the ArrayBuffer parts.

If there is a discussion to be had on the externalization feature, we can do that, but if it is a problem (which would surprise me), it also affects existing API.

[gabrielschulhof]

@addaleax if this does not go in as NAPI_EXPERIMENTAL then it'd have to go in as NAPI_VERSION 4.

[mhdawson]

@addaleax I think we'd agreed (at least on the N-API team) that everything would go in as experimental. We would then group things together and release in a new NAPI_VERSION as opposed to having every new function bump the VERSION. I think this will also minimize the chance that we accidentally release a new N-API function without bumping the NAPI_VERSION number.

[addaleax]

@mhdawson Tbh, that’s seems like very strong rigidity in the release process, but if that’s a rule the team has created, then feel free to ignore me

[Trott]

Does this need changes? Reviews? A CI run? Would love to see it not get stalled...

/ping @nodejs/n-api

[mhdawson]

It needs an update so that the new functions are being added as Experimental. (ie guarded by the experimental flag). I'd also like confirmation from @digitalinfinity that it is something that can be implemented by multiple different JavaScript engines.

[digitalinfinity]

I think we can support this one in Node-ChakraCore- @boingoing was going to take a look at adding a JSRT API for this (although by default SharedArrayBuffers are turned of in Chakra, we're looking at having it turned on in a scoped manner)

[Trott]

It needs an update so that the new functions are being added as Experimental. (ie guarded by the experimental flag).

@orange4glace Can you do that and rebase to eliminate conflicts?

[orange4glace]

@Trott Holy moly! I don't know if I did right?! -0- There were some major changes after my first PR. So I first synced my repo with upstream and added my codes again (c308106)

As @gabrielschulhof mentioned, typedarray also needs to be changed but that one is non-experimental and this one(sharedarraybuffer) is experimental so mixing them is maybe a kind of weird thing even though headers and (v8) implementations are separated.

[gabrielschulhof]

@orange4glace the implementation of N-API is always experimental. Only the APIs advertised in the header are separated into N-API versions 1-3, and NAPI_EXPERIMENTAL. So, you can add SharedArrayBuffer support to TypedArray unconditionally.

[gabrielschulhof]

This PR is heading in the right direction👍 It still needs the followings:

• documentation
• tests
• TypedArray support – which you can safely implement even though the API for SharedArrayBuffer is experimental, and the API for TypedArray is not.

[mhdawson]

@orange4glace the implementation of N-API is always experimental. Only the APIs advertised in the header are separated into N-API versions 1-3, and NAPI_EXPERIMENTAL. So, you can add SharedArrayBuffer support to TypedArray unconditionally.

I think we have to be careful here. I'm not sure what the right answer is but I think we need to avoid somebody using what they believe is N-API version 3 (which has TypedArray) and then later building against an earlier Node.js version and finding it does not work. I'll have to look at it in more detail to better understand if its an issue or not.

[gabrielschulhof]

@mhdawson you're right. Although the implementation never has NAPI_VERSION-keyed #ifdef blocks, adding functionality without changing the API surface at all (such as adding SharedArrayBuffer support to napi_create_typedarray and napi_get_typedarray_info) is also semver-minor, if we can use #21875 (the addition of the recursive mkdir flag) as a precedent because, although this modification of napi_create_typedarray has no new flags which are indicated explicitly, the JavaScript type of the buffer that would get passed into napi_create_typedarray would serve as an implicit flag which the code did not previously consult.

Thus, I don't think we can have napi_create_typedarray be stable in N-API version 3, while its SharedArrayBuffer functionality is experimental.

I think we need a napi_create_shared_typedarray to accompany the suite of SharedArrayBuffer APIs this PR introduces, the code of which is largely the same as that of napi_create_typedarray. Then the new SharedArrayBuffer API would be completely orthogonal to existing APIs.

[gabrielschulhof]

... and it could use the existing NAPI_EXPERIMENTAL -> some-version-n cadence we intend for all new N-APIs.

[gabrielschulhof]

There's still an issue we need to investigate. Faced with a TypedArray coming in from JavaScript, the addon author would need to know whether the TypedArray is backed by an ArrayBuffer or by a SharedArrayBuffer in order to call napi_get_typedarray_info vs. napi_get_shared_typedarray_info. How would an addon author who uses plain V8 proceed in such a case?

[gabrielschulhof]

@mhdawson some observations from running https://gist.github.com/gabrielschulhof/108166bc6e71a755caac6897be5b1f66 against Node.js 8.7.0 and 10.10:

• N-API treats typed arrays coming in from JavaScript correctly if they are created with a SharedArrayBuffer in JS and accessed with napi_get_typedarray_info, although
• Setting values in the void* data pointer coming from a SharedArrayBuffer doesn't seem to work.
• SharedArrayBuffer is not available or not exposed on Node.js v8.7.0, even though the API is available on the V8 side in Node.js 8.7.0:

  > const x = new SharedArrayBuffer(8);
  ReferenceError: SharedArrayBuffer is not defined
      at repl:1:11
      at ContextifyScript.Script.runInThisContext (vm.js:50:33)
      at REPLServer.defaultEval (repl.js:239:29)
      at bound (domain.js:301:14)
      at REPLServer.runBound [as eval] (domain.js:314:12)
      at REPLServer.onLine (repl.js:440:10)
      at emitOne (events.js:120:20)
      at REPLServer.emit (events.js:210:7)
      at REPLServer.Interface._onLine (readline.js:282:10)
      at REPLServer.Interface._line (readline.js:631:8)
  > 
  

V8 has 2 constructors each for all typed array types such that one accepts an ArrayBuffer and the other accepts a SharedArrayBuffer[0, 1]. Thus, we could conceivably loosen napi_create_typed_array to accept either an ArrayBuffer or a SharedArrayBuffer. I believe this does not break our promise of perpetual forward compatibility.

It looks like napi_get_typedarray_info correctly returns the napi_value containing the SharedArrayBuffer, but does not seem to correctly return the void* when the backing store is a SharedArrayBuffer -- or I simply made a mistake in my code, especially since it doesn't segfault or anything.

The cleanest solution though IMO is to introduce a second set of N-APIs that deal with SharedArrayBuffer-backed typed arrays (by adding napi_create_shared_typedarray and napi_get_shared_typedarray_info).

[gabrielschulhof]

Given that napi_get_typedarray_info works with SharedArrayBuffer-backed typed arrays, I think the solution addon authors can use for distinguishing between a SharedArrayBuffer-backed typed array and a plain ArrayBuffer-backed typed array coming in from JavaScript is to call napi_get_typedarray_info with a non-NULL parameter for the arraybuffer parameter and NULLs for every other out-parameter, then check the result of napi_is_arraybuffer on the resulting value. If it's true, continue using the napi_*_typedarray* methods. Otherwise start using napi_*_shared_typedarray* methods.

[Trott]

@nodejs/n-api @orange4glace Does someone want to pick this up and get it across the finish line?

[gireeshpunathil]

ping @orange4glace again

[orange4glace]

I can now work on this although a quite long time has been passed(Sorry for that! :P). Since the entire napi file structures are changed from the first PR was requested, it seems closing this and reopening a fresh PR might be good. Any ideas?

[gabrielschulhof]

@orange4glace no need to close this PR. It's fine if you just reset the branch and start pushing to it again.

[gabrielschulhof]

@orange4glace of course, closing it and starting a new one is fine too 🙂

[orange4glace]

In TypedArray, it seems there's some problem with V8 API to implement N-API functions.
Currently N-API has some TypedArray functions related with ArrayBuffer which are

napi_create_typedarray
napi_get_typedarray_info
napi_create_dataview
napi_is_dataview
napi_get_dataview_info

We can create TypedArray or DataView with SharedArrayBuffer by using corresponding V8 API which is for example,
static Local<DataView> DataView::New(Local<SharedArrayBuffer> shared_array_buffer, byte_offset, size_t length);
So there's no problem with implementing napi_create_typedarray or napi_create_dataview.

But, other functions like, for example napi_get_typedarray_info, since the underlying type of TyepdArray or DataView is ArrayBufferView and ArrayBufferView does not have a interface to retrieve its underlying SharedArrayBuffer. It only has ArrayBufferView::Local<ArrayBuffer> Buffer().
So it seems that there's no official way (with official API) to get the underlying SharedArrayBuffer instead of the ArrayBuffer although those two types are theoretically same JSArrayBuffer.

What's your thoughts? @gabrielschulhof

[jasnell]

This hasn't been updated in a while, is out of date. Closing but can reopen if it is picked back up again and updated.