Buffer support #13

RReverser · 2023-01-04T14:28:34Z

I don't understand the test structure - they seem to be adapted versions of official N-API tests, but without GYP and with some other modifications. Are they converted automatically somehow or do you rewrite them manually?

Also, you might notice that I went the slightly easier path for napi_create_external_buffer by simply wrapping napi_create_external_arraybuffer. In theory, both should have more or less the same effect, except that array buffer might be reused for other views and so the finalize callback might be called a bit later; that said, finalize callbacks are already not guaranteed to run at specific time (or at all) so this shouldn't be an issue.

RReverser · 2023-01-04T15:04:26Z

~~Testing manually for now, looks like I missed some stuff. Fixing.~~ Done.

toyobayashi · 2023-01-05T02:47:02Z

Thank you for your contribution.

Are they converted automatically somehow or do you rewrite them manually?

Honestly, I don't know how to use node-gyp to build emscripten project. To use the official test cases, I manually copied them to this repository, then used my toy project @tybys/cgen as the build tool, and I could use JavaScript to write the configuration (cgen.config.js) for the build. Also, since wasm loading is asynchronous, we can not directly use the require function, so I modified the code for initialization.

I will add official buffer test here later.

RReverser · 2023-01-05T03:12:09Z

Honestly, I don't know how to use node-gyp to build emscripten project.

Heh, I actually have separate upcoming PR for that to emnapi :)

I will add official buffer test here later.

Thanks!

RReverser · 2023-01-05T14:31:58Z

Ok fixed the problems found via tests (btw, can you enable CI for me please?).

One thing I'm not happy about and couldn't figure out is how upstream N-API implementation gets away with not truncating the NULL terminator from the char array when creating a same-sized Buffer: f043338 (#13)

That is, without me doing this -1 thing, all the buffers would have final null terminator included in the size and the tests would fail because .toString() would return Lorem ipsum...\0 instead of just Lorem ipsum.... And, logically, that seems expected, but given that this test works somehow in upstream Node, I guess they're doing something special?

toyobayashi · 2023-01-05T16:09:11Z

Oh, It's napi_create_string_utf8 bug here.

Without -1, binding.theText missing the \0 in emnapi test, but include \0 in native.

Seems we can not use UTF8ToString if length !== -1

RReverser · 2023-01-05T16:44:55Z

Ah... yes, an old problem :( emscripten-core/emscripten#4693

RReverser · 2023-01-05T17:06:32Z

Ok I removed the - 1 from the test to avoid accidentally hiding that bug, now that we know what it is.

toyobayashi · 2023-01-06T02:43:23Z

Before merge this PR, I have two questions ask for your opinion:

Currently napi_create_external_buffer calls napi_create_external_buffer, means that the JS Buffer's memory is a copy from wasm void* data. While emnapi_create_external_uint8array create a view from wasm memory without copying. Is it better to do the similar things in napi_create_external_buffer to avoid user calling emnapi_sync_memory?
napi_is_buffer is not wrapped by $PREAMBLE! (so does Node.js source code), it will throw ReferenceError if there is no window.Buffer in browser instead of returning a napi_status, while Node.js napi_is_buffer never throws, is it better to make its behavior more closer to Node.js?

RReverser · 2023-01-06T03:02:43Z

Currently napi_create_external_buffer calls napi_create_external_buffer, means that the JS Buffer's memory is a copy from wasm void* data. While emnapi_create_external_uint8array create a view from wasm memory without copying. Is it better to do the similar things in napi_create_external_buffer to avoid user calling emnapi_sync_memory?

I briefly thought about this, but there are two problems:

I still don't understand finalization helpers in this codebase well enough 😅 (and, unfortunately, they're not very commented / documented, so that didn't help).
I realised that emnapi_sync_memory will be necessary a bit more rarely, but it will be still necessary whenever Wasm memory is resized due to some reallocation. In fact, now that you mentioned it, I'm pretty sure that emnapi_create_external_uint8array will suffer from the same problem (unless you already support synchronising it too?).

If you reallocate, it triggers memory.grow(...) on Wasm, which, in turn, invalidates the underlying ArrayBuffer, and that, in turn, invalidates any views into it, including the view returned from emnapi_create_external_uint8array. There are some future proposals that will make it easier, but for now you need to manually re-create views whenever that happens - e.g. that's exactly what Emscripten & wasm-bindgen do to keep things like HEAPU8, HEAPU16 etc. working.

So, yes, I do think that switching to Buffer being a view would be useful - at least it would certainly be faster due to avoiding a copy - but (an enhanced version of) emnapi_sync_memory will be still necessary.

2. napi_is_buffer is not wrapped by $PREAMBLE! (so does Node.js source code), it will throw ReferenceError if there is no window.Buffer in browser instead of returning a napi_status, while Node.js napi_is_buffer never throws, is it better to make its behavior more closer to Node.js?

Ah yeah, good point - I didn't think of the missing Buffer in browser. I think just adding typeof Buffer === 'function' should be enough, ~~let me do that~~ done.

RReverser · 2023-01-06T03:22:04Z

2. I'm pretty sure that emnapi_create_external_uint8array will suffer from the same problem (unless you already support synchronising it too?).

Yeah confirmed with a simple failing test: RReverser@b44912b. Feel free to cherry-pick if you want to make it work with emnapi_sync_memory somehow.

toyobayashi · 2023-01-06T03:32:29Z

it will be still necessary whenever Wasm memory is resized due to some reallocation

Nice, didn't realize it, thanks for pointing this out. I will try to make it work.

toyobayashi · 2023-01-05T04:01:16Z

packages/emnapi/src/value/convert2c.ts

+  if (!handle.isBuffer()) {
+    return envObject.setLastError(napi_status.napi_invalid_arg)
+  }
+  return napi_get_typedarray_info(env, buffer, 0, length, data, 0, 0)


imported symbols start with _ at runtime

toyobayashi · 2023-01-05T04:09:42Z

packages/emnapi/src/value/create.ts

+    const value = emnapiCtx.addToCurrentScope(arrayBuffer).id
+    $makeSetValue('result', 0, 'value', '*')


value is renamed here by macro expanding, cause makeSetValue receive a wrong variable. It should be declared outside of $PREAMBLE! macro.

The return value is an ArrayBuffer, but nodejs Buffer is a Uint8Array.

toyobayashi · 2023-01-05T04:14:37Z

packages/emnapi/src/value/create.ts

+function napi_create_external_buffer(
+  env: napi_env,
+  length: size_t,
+  data: Pointer<void>,
+  finalize_cb: napi_finalize,
+  finalize_hint: Pointer<void>,
+  result: Pointer<napi_value>
+) {
+  $PREAMBLE!(env, (envObject) => {
+    const status = napi_create_external_arraybuffer(env, data, length, finalize_cb, finalize_hint, result)
+    if (status !== napi_status.napi_ok) {
+      return status
+    }
+    const handleId = $makeGetValue('result', 0, '*')
+    const handle = emnapiCtx.handleStore.get(handleId)!
+    // I know I'm the only owner of handle, so just wrap value in-place.
+    handle.value = Buffer.from(handle.value)
+    return envObject.getReturnStatus()
+  })
+}


Should we do something like emnapi_create_external_uint8array here to avoid memory copying?

toyobayashi · 2023-01-05T04:19:59Z

packages/runtime/src/Handle.ts

@@ -63,6 +63,10 @@ export class Handle<S> {
    return !this.isEmpty() && (ArrayBuffer.isView(this.value)) && !(this.value instanceof DataView)
  }

+  public isBuffer (): boolean {
+    return !this.isEmpty() && (this.value instanceof Buffer)


There is no globalThis.Buffer in browser environment, prefer typeof Buffer !== 'undefined' && Buffer.isBuffer(this.value)

RReverser · 2023-01-07T00:14:09Z

Hm, little confused at these comments - they seem to be the ones I already found & fixed but Github shows that they were left after the PR was merged?

toyobayashi · 2023-01-07T13:02:46Z

emm, sorry, probably it was my bad, I left those comments but forgot to clicked "submit review" button at first, and clicked approve before merging. I thought you had seen it.

RReverser · 2023-01-07T13:15:23Z

emm, sorry, probably it was my bad, I left those comments but forgot to clicked "submit review" button at first, and clicked approve before merging. I thought you had seen it.

Ah, no, I just ran into same bugs on my own after you added tests, and fixed them myself 😅

RReverser force-pushed the buffer branch 2 times, most recently from 0115d98 to d0c2923 Compare January 4, 2023 14:53

Buffer support

fd99706

RReverser force-pushed the buffer branch from d0c2923 to fd99706 Compare January 4, 2023 14:55

RReverser added 2 commits January 4, 2023 15:28

More Buffer methods & fixes

bb44bf6

Fix createArrayBuffer registration

e78fe3b

toyobayashi and others added 2 commits January 5, 2023 11:26

test: add buffer test

c2dd1ca

Various fixes

f043338

Remove the -1 from text length

dabe062

Check for Buffer presence

67dfc49

This was referenced Jan 6, 2023

emnapi_create_external_uint8array should integrate with emnapi_sync_memory #16

Closed

napi_create_string_utf8 should preserve null terminator #17

Closed

toyobayashi approved these changes Jan 6, 2023

View reviewed changes

toyobayashi merged commit b2946a5 into toyobayashi:main Jan 6, 2023

RReverser deleted the buffer branch January 7, 2023 00:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer support #13

Buffer support #13

RReverser commented Jan 4, 2023

RReverser commented Jan 4, 2023 •

edited

Loading

toyobayashi commented Jan 5, 2023

RReverser commented Jan 5, 2023

RReverser commented Jan 5, 2023

toyobayashi commented Jan 5, 2023 •

edited

Loading

RReverser commented Jan 5, 2023

RReverser commented Jan 5, 2023

toyobayashi commented Jan 6, 2023

RReverser commented Jan 6, 2023 •

edited

Loading

RReverser commented Jan 6, 2023 •

edited

Loading

toyobayashi commented Jan 6, 2023

toyobayashi Jan 5, 2023

toyobayashi Jan 5, 2023

toyobayashi Jan 5, 2023

toyobayashi Jan 5, 2023

RReverser commented Jan 7, 2023

toyobayashi commented Jan 7, 2023

RReverser commented Jan 7, 2023

		const value = emnapiCtx.addToCurrentScope(arrayBuffer).id
		$makeSetValue('result', 0, 'value', '*')

Buffer support #13

Buffer support #13

Conversation

RReverser commented Jan 4, 2023

RReverser commented Jan 4, 2023 • edited Loading

toyobayashi commented Jan 5, 2023

RReverser commented Jan 5, 2023

RReverser commented Jan 5, 2023

toyobayashi commented Jan 5, 2023 • edited Loading

RReverser commented Jan 5, 2023

RReverser commented Jan 5, 2023

toyobayashi commented Jan 6, 2023

RReverser commented Jan 6, 2023 • edited Loading

RReverser commented Jan 6, 2023 • edited Loading

toyobayashi commented Jan 6, 2023

toyobayashi Jan 5, 2023

Choose a reason for hiding this comment

toyobayashi Jan 5, 2023

Choose a reason for hiding this comment

toyobayashi Jan 5, 2023

Choose a reason for hiding this comment

toyobayashi Jan 5, 2023

Choose a reason for hiding this comment

RReverser commented Jan 7, 2023

toyobayashi commented Jan 7, 2023

RReverser commented Jan 7, 2023

RReverser commented Jan 4, 2023 •

edited

Loading

toyobayashi commented Jan 5, 2023 •

edited

Loading

RReverser commented Jan 6, 2023 •

edited

Loading

RReverser commented Jan 6, 2023 •

edited

Loading