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

node-api: update headers for better wasm support #49037

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

toyobayashi
Copy link
Contributor

@toyobayashi toyobayashi commented Aug 6, 2023

Hi, Node-API team @nodejs/node-api

I'm wondering if emnapi could use node-api-headers package instead of maintaining a modified version of headers.

Just a little modification:

  • change __wasm32__ to __wasm__ for building wasm64-unknown-emscripten

  • expand NAPI_MODULE_EXPORT to __attribute__((used)) (EMSCRIPTEN_KEEPALIVE) to export napi_register_wasm_v1 and node_api_module_get_api_version_v1 if build with Emscripten

  • expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten. Not sure if this should. Currently there is no way to specify a custom module name in wasm import object for imported function which is implemented in JavaScript, all functions imported from JavaScript are under env module by default. If don't do this change, emnapi must require user to provide a instantiateWasm hook to emscripten and add napi module during this hook call, which isn't what instantiateWasm are supposed to be used for, that force users to write loading logic themselves.

    import init from './node-api-wasm-module-compiled-by-emcc.js'
    
    await init({
      instantiateWasm (imports, successCallback) {
        imports.napi = imports.env
        WebAssembly.instantiate(..., imports).then(({ instance, module }) => {
          successCallback(instance, module)
        })
        return {}
      }
    })
  • remove __wasm32__ guards on async work and TSFN, because they have been implemented and are available in wasm, even in wasm32-unknown-unknown and wasm32-wasi. It is worth mentioning that I created a PR src: define NAPI_HAS_THREADS to make TSFN available on Emscripten node-addon-api#1283 that added NAPI_HAS_THREADS to Napi::AsyncWorker and Napi::ThreadSafeFunction, now maybe just add it to Napi::AsyncProgressWorker is enough because it's using std::mutex, which requires __EMSCRIPTEN_PTHREADS__ or __wasi__ && _REENTRANT

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Aug 6, 2023
@mhdawson
Copy link
Member

@toyobayashi what is the likelyhood that this would affect any existing addons?

@mhdawson
Copy link
Member

@cjihrig I'm wondering if you have any thoughts/opinions on the request related to expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten

@cjihrig
Copy link
Contributor

cjihrig commented Aug 14, 2023

@mhdawson I don't think I'm familiar enough with this work to have a good opinion.

@devsnek
Copy link
Member

devsnek commented Aug 14, 2023

expand NAPI_EXTERN to __attribute__((__import_module__("env"))) if build with Emscripten

This very intentionally already expands to __attribute__((__import_module__("napi"))).

What we could do instead is allow you to define your own NAPI_EXTERN with a simple ifndef check. The default namespace would still be napi but you could modify it on a per-build basis.

@toyobayashi
Copy link
Contributor Author

what is the likelyhood that this would affect any existing addons?

@mhdawson Reverted changes on NAPI_EXTERN. I believe the remaining changes will have no impact on existing addons.

@toyobayashi
Copy link
Contributor Author

toyobayashi commented Aug 15, 2023

define your own NAPI_EXTERN with a simple ifndef check

Should we add #ifndef NAPI_EXTERN in BUILDING_NODE_EXTENSION? if BUILDING_NODE_EXTENSION is defined (node-gyp default behavior), custom NAPI_EXTERN seems to not work?

node/src/node_api.h

Lines 4 to 11 in 0d03b77

#ifdef BUILDING_NODE_EXTENSION
#ifdef _WIN32
// Building native addon against node
#define NAPI_EXTERN __declspec(dllimport)
#elif defined(__wasm__)
#define NAPI_EXTERN __attribute__((__import_module__("napi")))
#endif
#endif

- #ifdef BUILDING_NODE_EXTENSION
+ #if defined(BUILDING_NODE_EXTENSION) && !defined(NAPI_EXTERN)

Sharp is using node-gyp, emnapi and emscripten to build wasm port, the wasm version is already available on StackBlitz WebContainer. Though #ifndef NAPI_EXTERN already exists in js_native_api.h, I think adding && !defined(NAPI_EXTERN) to #ifdef BUILDING_NODE_EXTENSION is still necessary here since it comes before including js_native_api.h. Otherwise would request users who use both node-gyp and emscripten to undef BUILDING_NODE_EXTENSION in addition to define their own NAPI_EXTERN.

@gabrielschulhof
Copy link
Contributor

It makes sense that, if js_native_api.h is included via node_api.h, then NAPI_EXTERN should still be used if already available.

@mhdawson
Copy link
Member

@legendecas is going to take another look at this before we land it.

@legendecas legendecas added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit b55adfb into nodejs:main Oct 5, 2023
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b55adfb

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
PR-URL: nodejs#49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#49037
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants