Skip to content
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 SharedArrayBuffer api #23279

Closed
wants to merge 7 commits into from
Closed

Conversation

orange4glace
Copy link

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.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Oct 5, 2018
@orange4glace
Copy link
Author

Build failed

@aqrln
Copy link
Contributor

aqrln commented Oct 5, 2018

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

@orange4glace
Copy link
Author

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
Copy link
Member

addaleax commented Oct 5, 2018

@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?

@addaleax addaleax reopened this Oct 5, 2018
@orange4glace
Copy link
Author

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
Copy link
Member

addaleax commented Oct 5, 2018

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

@gabrielschulhof
Copy link
Contributor

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

src/node_api.cc Outdated
return GET_RETURN_STATUS(env);
}

napi_status napi_create_external_sharedarraybuffer(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek Just for context, we also have napi_create_external_arraybuffer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@orange4glace orange4glace Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2018

@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
Copy link
Member

mhdawson commented Oct 5, 2018

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

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2018

@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
Copy link
Author

orange4glace commented Oct 5, 2018

@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
Copy link
Member

addaleax commented Oct 5, 2018

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
Copy link
Contributor

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

@mhdawson
Copy link
Member

mhdawson commented Oct 9, 2018

@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
Copy link
Member

@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
Copy link
Member

Trott commented Nov 16, 2018

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

/ping @nodejs/n-api

@mhdawson
Copy link
Member

mhdawson commented Nov 16, 2018

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
Copy link
Contributor

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
Copy link
Member

Trott commented Nov 18, 2018

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
Copy link
Author

orange4glace commented Nov 18, 2018

@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
Copy link
Contributor

@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.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@gabrielschulhof
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

Trott commented Oct 23, 2019

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

@fhinkel fhinkel added the stalled Issues and PRs that are stalled. label Oct 28, 2019
@gireeshpunathil
Copy link
Member

ping @orange4glace again

@orange4glace
Copy link
Author

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
Copy link
Contributor

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

@gabrielschulhof
Copy link
Contributor

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

@orange4glace
Copy link
Author

orange4glace commented Dec 29, 2019

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

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.