node-api: add napi_create_buffer_from_arraybuffer method

[mertcanaltin]

introduces a new N-API method napi_create_buffer_from_arraybuffer that allows the creation of a Buffer from an ArrayBuffer. This new method provides functionality similar to napi_create_typedarray, enabling developers to specify a byte offset and length to create a Buffer from a portion of the ArrayBuffer. This feature addresses the limitation where it was previously not possible to create a Buffer directly from an ArrayBuffer using N-API.

Closes: #54440

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/node-api

[mertcanaltin]

I made make in my local environment and ran the make only-tesy command and saw that it passed the build and tests, I will check the places that give errors again.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 88.41%. Comparing base (aac8ba7) to head (6c4e386). Report is 797 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_api.cc	50.00%	3 Missing and 5 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54505      +/-   ##
==========================================
+ Coverage   88.24%   88.41%   +0.16%     
==========================================
  Files         651      652       +1     
  Lines      183856   186628    +2772     
  Branches    35853    36065     +212     
==========================================
+ Hits       162251   165001    +2750     
+ Misses      14896    14885      -11     
- Partials     6709     6742      +33     

Files with missing lines	Coverage Δ
src/node_api.cc	76.26% <50.00%> (-0.53%)	:arrow_down:

... and 135 files with indirect coverage changes

[legendecas]

@ronag would you mind sharing a use case for this method? Does https://nodejs.org/api/n-api.html#napi_create_typedarray work for the use case?

We talked about this in Node-API meeting, a new node-api can not be easily changed or deprecated, a concrete use case would really help us understand the necessity to add the new API and if the API interface sufficient.

[ronag]

@ronag would you mind sharing a use case for this method? Does https://nodejs.org/api/n-api.html#napi_create_typedarray work for the use case?

We talked about this in Node-API meeting, a new node-api can not be easily changed or deprecated, a concrete use case would really help us understand the necessity to add the new API and if the API interface sufficient.

It doesn't work if I need to return node buffers in the js api. I guess I could change the prototype in js but that seems a bit ugly.

[ronag]

@nodejs/node-api

[targos]

@ronag would you mind sharing a use case for this method? Does nodejs.org/api/n-api.html#napi_create_typedarray work for the use case? We talked about this in Node-API meeting, a new node-api can not be easily changed or deprecated, a concrete use case would really help us understand the necessity to add the new API and if the API interface sufficient.

It doesn't work if I need to return node buffers in the js api. I guess I could change the prototype in js but that seems a bit ugly.

• In C: napi_create_typedarray
• Then in JS: Buffer.from(array.buffer, array.byteOffset, array.byteLength)

[ronag]

Creating buffer instances is super slow so I don't think that's an option.

[targos]

This is a special case where after a few type checks it directly creates a FastBuffer

[ronag]

Yes, creating Uint8Arrays is super slow. I'd like to avoid creating 2 for each instance. I have a workaround where I pass 2 array buffers (data and sizes) to js but it's hacky

[ronag]

So currently I have this situation:

1. For returning buffers from an API. I create two array buffers, one containing all the buffer data and one containing the buffer sizes (as 32 bit integers) and then I create the Buffer's from these in JS.

2. For string I have the opposite problem. Converting buffers to strings in JS is slow. So I should create them in native code.

I'm happy to jump into a napi working group meeting to further discuss.

[legendecas]

I'm happy to jump into a napi working group meeting to further discuss.

It would be awesome!

Is https://github.com/nxtedition/rocks-level/pull/16 a benchmark in real world case?

[ronag]

I'm happy to jump into a napi working group meeting to further discuss.

It would be awesome!

Is nxtedition/rocks-level#16 a benchmark in real world case?

If you can put this on the agenda I could join on Friday?

[toyobayashi]

Buffer is Node.js specific, is it better to place this API in node_api.h like napi_is_buffer?

[legendecas]

We'll go through PRs labeled with node-api in the weekly meetings. This is already on agenda.

[vmoroz]

We are going to discuss this PR this Friday, but I want to point out that the js_native_api.h is created for the Node.js and JS engine independent APIs. All Node.js specific APIs must go into the node_api.h, the related implementation to the node_api.cc, and all the related unit tests to the test/node-api folder. E.g. you can find the napi_create_buffer function in the node_api.h file because the Buffer is a Node.js specific object.

Also, all new Node-APIs must be prefixed with the node_api_ instead of napi_. I believe it is related to the napi word to be an offensive word in some context. (I wonder why C++ does not rename std namespace for the same reasons. :) )

More guidance could be found here: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md We must update it with the points listed above.

[ronag]

Then we might as well call this node_create_buffer?

[NickNaso]

Then we might as well call this node_create_buffer?

based on the @vmoroz suggestion it should be named node_api_create_buffer_from_arraybuffer.

[vmoroz]

Then we might as well call this node_create_buffer?

based on the @vmoroz suggestion it should be named node_api_create_buffer_from_arraybuffer.

@ronag , we already have the napi_create_buffer function name. The name node_create_buffer sounds almost the same except for the prefix. It may cause unnecessary confusion for users. We also did not use the node_ prefix yet. It does not seem to be worth to introduce the 3rd prefix type just for this one case.

Though, generally I like the shorter prefix node_ instead of the node_api_. E.g. we could use the node_ prefix for Node.js specific APIs and js_ or jsr_ (short for JS Runtime) prefix for the generic JS engine APIs. But this is a much bigger change that must be discussed separately. There was a previous discussion here after the N-API was renamed to Node-API.

For now lets stick with the node_api_create_buffer_from_arraybuffer name proposed by @NickNaso.

[mertcanaltin]

I applied it as you said, I will send it today, thank you very much

[ronag]

Here is a possible motivating sample:

Creating buffers from pointers can be slow as each pointer requires a finalizer to be registered.

static void Finalize(napi_env env, void* data, void* hint) {
  if (hint) {
    delete reinterpret_cast<std::shared_ptr<std::vector<uint8_t>>*>(hint);
  }
}

napi_value make_buffers(napi_env env, std::vector<size_t> sizes, std::vector<uint8_t> data) {
  napi_value result;
  NAPI_STATUS_RETURN(napi_create_array_with_length(env, sizes.size(), &result));

  auto data = std::make_shared<std::vector<uint8_t>>(std::move(data));
  auto ptr = data.data();
  for (auto n = 0; n < count; n++) {
    const auto size = sizes[n];

    napi_value row;
    NAPI_STATUS_RETURN(napi_create_external_buffer(
      env,
      ptr,
      size,
      Finalize,
      new std::shared_ptr<std::vector<uint8_t>>(data),
      &row
    ));
    ptr += size;

    NAPI_STATUS_RETURN(napi_set_element(env, result, n, row));
  }

  return result;
}

A faster alternative is to create a single ArrayBuffer and then slice it into multiple Buffers, hence only one finalizer is registered.

static void Finalize(napi_env env, void* data, void* hint) {
  if (hint) {
    delete reinterpret_cast<std::vector<uint8_t>*>(hint);
  }
}

napi_value make_buffers(napi_env env, std::vector<size_t> sizes, std::vector<uint8_t> data) {
  napi_value result;
  NAPI_STATUS_RETURN(napi_create_array_with_length(env, sizes.size(), &result));

  napi_value arraybuffer;
  {
    // We create one ArrayBuffer and then slice it into Buffer(s) to avoid the
    // overhead of registering lots of finalizers which is slow.
    auto data = std::make_unique<std::vector<uint8_t>>(std::move(data));
    NAPI_STATUS_RETURN(napi_create_external_arraybuffer(
      env,
      data->data(),
      data->size(),
      Finalize,
      data.get(),
      &arraybuffer
    ));
    data.release();
  }

  auto offset = 0;
  for (auto n = 0; n < count; n++) {
    const auto size = sizes[n];

    napi_value row;
    NAPI_STATUS_RETURN(napi_create_buffer_from_array_buffer(
      env,
      size,
      arraybuffer,
      offset,
      &row
    ));
    offset += size;

    NAPI_STATUS_RETURN(napi_set_element(env, result, n, row));
  }
}

[mertcanaltin]

Thank you very much for the review and suggestions @vmoroz

[vmoroz]

Thank you very much for the review and suggestions @vmoroz

@mertcanaltin , thank you for addressing the PR feedback.

I guess that you are still working on the unit tests: there must be changes to the test JS code to invoke the new native test method.

BTW, the test_general seems to be not the best place for the new unit test. Should it be in test\node-api\test_buffer next to other Buffer API tests?

[legendecas]

Would you mind fixing the test failure? Thanks!

[vmoroz]

@mertcanaltin , it seems that the build error is related to the NAPI_EXPERIMENTAL and that it uses the new "synchronous" finalizers (the finalizers do not have to wait until the next event loop tick). Let me know if you need help with fixing it. The new finalizers cannot run any JS related code as they run while GC is in an unstable state. To invoke JS affecting code from the finalizers developers can use the new node_api_post_finalizer in the finalizer and pass all the code as a callback to that function. This code will be executed in the next event loop tick as it used to be before.

[mertcanaltin]

@mertcanaltin , it seems that the build error is related to the NAPI_EXPERIMENTAL and that it uses the new "synchronous" finalizers (the finalizers do not have to wait until the next event loop tick). Let me know if you need help with fixing it.

The new finalizers cannot run any JS related code as they run while GC is in an unstable state.

To invoke JS affecting code from the finalizers developers can use the new node_api_post_finalizer in the finalizer and pass all the code as a callback to that function. This code will be executed in the next event loop tick as it used to be before.

I did a bit update, thank you very much for your suggestion

[vmoroz]

Just to be on the same page: my suggestion regarding the use of node_api_post_finalizer was in the context of the failing tests caused by the new NAPI_EXPERIMENTAL in the gyp file. The malignDeleter function in the test_finalizer.c file calls functions like napi_get_global that cause the crash in the new immediate finalizers while running tests. To address this issue the code in the malignDeleter function must be changed to a call to a single function node_api_post_finalizer which would call a new function with the body of the former malignDeleter function.

[mertcanaltin]

Just to be on the same page: my suggestion regarding the use of node_api_post_finalizer was in the context of the failing tests caused by the new NAPI_EXPERIMENTAL in the gyp file. The malignDeleter function in the test_finalizer.c file calls functions like napi_get_global that cause the crash in the new immediate finalizers while running tests. To address this issue the code in the malignDeleter function must be changed to a call to a single function node_api_post_finalizer which would call a new function with the body of the former malignDeleter function.

I made an update @vmoroz

[mertcanaltin]

I wonder if you have a suggestion for incompatible signatures here, I tried const but I got an error again

gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
  CC(target) Release/obj.target/test_buffer/test_buffer.o
../test_buffer.c:47:42: warning: incompatible function pointer types passing 'void (napi_env, void *, void *)' (aka 'void (struct napi_env__ *, void *, void *)') to parameter of type 'node_api_basic_finalize' (aka 'void (*)(const struct napi_env__ *, void *, void *)') [-Wincompatible-function-pointer-types]
          env, sizeof(theText), theCopy, deleteTheText,
                                         ^~~~~~~~~~~~~
../../../js-native-api/common.h:90:27: note: expanded from macro 'NODE_API_CALL'
  NODE_API_CALL_BASE(env, the_call, NULL)
                          ^~~~~~~~
../../../js-native-api/common.h:74:10: note: expanded from macro 'NODE_API_CALL_BASE'
    if ((the_call) != napi_ok) {                                         \
         ^~~~~~~~
/Users/mert/Library/Caches/node-gyp/20.17.0/include/node/node_api.h:134:53: note: passing argument to parameter 'finalize_cb' here
                            node_api_basic_finalize finalize_cb,
                                                    ^
../test_buffer.c:106:49: warning: incompatible function pointer types passing 'void (napi_env, void *, void *)' (aka 'void (struct napi_env__ *, void *, void *)') to parameter of type 'node_api_basic_finalize' (aka 'void (*)(const struct napi_env__ *, void *, void *)') [-Wincompatible-function-pointer-types]
          env, sizeof(theText), (void*)theText, noopDeleter,
                                                ^~~~~~~~~~~
../../../js-native-api/common.h:90:27: note: expanded from macro 'NODE_API_CALL'
  NODE_API_CALL_BASE(env, the_call, NULL)
                          ^~~~~~~~
../../../js-native-api/common.h:74:10: note: expanded from macro 'NODE_API_CALL_BASE'
    if ((the_call) != napi_ok) {                                         \
         ^~~~~~~~
/Users/mert/Library/Caches/node-gyp/20.17.0/include/node/node_api.h:134:53: note: passing argument to parameter 'finalize_cb' here
                            node_api_basic_finalize finalize_cb,
                                                    ^
2 warnings generated.
  SOLINK_MODULE(target) Release/test_buffer.node
  CC(target) Release/obj.target/test_finalizer/test_finalizer.o
  SOLINK_MODULE(target) Release/test_finalizer.node
gyp info ok 

[legendecas]

The type of the first argument of a finalizer with NAPI_EXPERIMENTAL should be node_api_basic_env: https://nodejs.org/api/n-api.html#node_api_basic_finalize, and per suggestion https://github.com/nodejs/node/pull/54505#issuecomment-2369057872 for `node_api_post_finalizer.