-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
ArrayBuffer and Binary json encoding #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Your changes look good. I have a suggestion for improving the efficiency of copying a ArrayBuffer into QuickJS, and it would also be nice to see some tests.
* | ||
* **WARNING**: QuickJS's binary JSON doesn't have a standard so expect it to change between version | ||
*/ | ||
encodeBinaryJSON(handle: QuickJSHandle): QuickJSHandle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to include a comment in the code showing how to use this that shows up in the documentation. How would I persist or transfer this value over the network? What's the use-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly have been using decodeBinaryJSON to handle IPC between an app that uses quickJS and one that uses node.js. However, I realized that encodeBinaryJSON isn't all that useful without some sort of getArrayBuffer function. I've been trying to write one, but it seems to not want to work.
c/interface.c
Outdated
} | ||
|
||
// -------------------- | ||
// Debugging QuickJS values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't make sense to me
c/interface.c
Outdated
@@ -301,6 +301,10 @@ JSValue *QTS_NewArray(JSContext *ctx) { | |||
return jsvalue_to_heap(JS_NewArray(ctx)); | |||
} | |||
|
|||
JSValue *QTS_NewArrayBuffer(JSContext *ctx, JSVoid *buffer, size_t length) { | |||
return jsvalue_to_heap(JS_NewArrayBufferCopy(ctx, (uint8_t*)buffer, length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method always does an extra copy:
- Copy from source ArrayBuffer to Emscripten memory at the _malloc pointer
- Copy from the _malloc pointer to the quickjs ArrayBuffer
We can eliminate copy 2 by using JS_NewArrayBuffer
to take ownership of the already malloc'd WebAssembly memory. You'd need to define a JSFreeArrayBufferDataFunc, but it'd be super simple, something like void qts_free_buffer(JSRuntime *unused_rt, void *unused_opaque, void *ptr) { free(ptr) }
. Then skip wrapping the malloc pointer in a lifetime and you're good to go.
I added a getArrayBuffer function, but the values outputed by it seem to be incorrect for unknown reasons, so we should put this into draft until that can be fixed |
If the array buffer creation bit works well, can we merge that without the JSONB parts? |
After some experiments, I noticed that NewArrayBufferCopy and NewArrayBuffer creates different outputs when using GetArrayBuffer. I'm guessing you are supposed to read the buffer differently but not sure how the code knows the difference and how it reads NewArrayBuffer. Using NewArrayBufferCopy fixes all the issues with GetArrayBuffer. |
ts/context.ts
Outdated
.newHeapBufferPointer(array) | ||
.consume((bufferHandle) => this.ffi.QTS_NewArrayBuffer(this.ctx.value, bufferHandle.value.pointer, array.length)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing this line seemed to fix the issue. Not sure if it'll cause more issues, but it seems to be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to see some tests for this stuff. Other than that, it looks ready to merge.
// I don't know how to return two values in C, maybe allocate memory in stack? | ||
size_t QTS_GetArrayBufferLength(JSContext *ctx, JSValueConst *data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could add stackAlloc
and use it to pass in a *size_t
argument like the &length
arguments in QuickJS's own API, but it's not document anywhere so I think your current approach is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking online, the best solutions I could find is to use Emscripten's embind API and a separate cpp file to use the API. However, I'm not familiar with Makefile, so I don't really know how to get it to compile the separate cpp file.
I can find what the command should look like:
$ emcc --bind -O3 --std=c++11 a_c_file.c another_c_file.c -x c++ your_cpp_file.cpp
But not how to do it using Make because I get this error
emcc: error: cannot specify -o with -c/-S/-E/-M and multiple source files
Removing -c gets me this error
wasm-ld: error: unknown file type: build/wrapper/interface.WASM_DEBUG_SYNC.o
Also updated the docs files
56146eb
to
b6a8a04
Compare
It still says awaiting for approval |
I also came across a use case where I need to share an ArrayBuffer between host and client, it'd be great to see this merged in! |
I has the same scenario, but i went some issues. javascript code executed in quickjs console.log(new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1])); When i use |
I needed this for a project, and so I implemented them for myself. However, I noticed that others asked about it here, #68, so I decided to share them.
Example: