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

Update JS_NewTypedArray() to C level API #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xeioex
Copy link
Contributor

@xeioex xeioex commented May 9, 2024

No description provided.

@xeioex xeioex force-pushed the updated-api-typed-array-ctor branch from 179a58a to 29e1525 Compare May 9, 2024 20:43
@saghul
Copy link
Contributor

saghul commented May 10, 2024

For strings we have an api that copies data and one that doesn't. Should we have the same here? /cc @chqrlie

@chqrlie
Copy link
Collaborator

chqrlie commented May 10, 2024

For strings we have an api that copies data and one that doesn't. Should we have the same here? /cc @chqrlie

What string APIs are you referring to?

Regarding typed arrays, the APIs should either create a view on an existing ArrayBuffer / SharedArrayBuffer or create the full view of an ArrayBuffer implicitly created from raw data. I am not sure what is the best way to approach this.

What are the needs for integrating QuickJS into larger applications ?

@saghul
Copy link
Contributor

saghul commented May 10, 2024

Sorry I think I was thing about arraybuffer.

In txiki.js I have both use cases, but the one I use the most is the one that doesn't copy. This is what I use for sockets, stdio and so on.

@kasperisager
Copy link
Contributor

kasperisager commented May 10, 2024

My own use case is replacing the implementation of this function: https://github.com/holepunchto/libqjs/blob/eff318a3c97839b55880bc52cc92ee6528005811/src/qjs.c#L2846-L2891. The function behaves the same as the various typed array constructors in that it takes an arraybuffer, an offset, and a length and constructs a view over that.

@chqrlie
Copy link
Collaborator

chqrlie commented May 10, 2024

My own use case is replacing the implementation of this function: https://github.com/holepunchto/libqjs/blob/eff318a3c97839b55880bc52cc92ee6528005811/src/qjs.c#L2846-L2891. The function behaves the same as the various typed array constructors in that it takes an arraybuffer, an offset, and a length and constructs a view over that.

@kasperisager in your case, you want to bypass the overhead of JS_GetGlobalObject(env->context); and the appropriate JS_GetPropertyStr(env->context, global, class) but you would still potentially need to map your js_typedarray_type_t enum to QuickJS's new JSTypedArrayEnum.

What is js_typedarray_type_t enum modelled after ? I could not find an equivalent in v8.
Reordering our own enum should not be a problem (yet).

2 separate APIs seem to be advisable.

@saghul
Copy link
Contributor

saghul commented May 10, 2024

This is the little wrapper I use: https://github.com/saghul/txiki.js/blob/4896636f13dd298468c9f6b3d46cbc0dcbab12b3/src/utils.c#L262 since JS_NewUint8Array does not copy the data but needs a freeing function.

When reading data from a socket with libuv I allocate some memory and then that memory becomes the uint8array.

@kasperisager
Copy link
Contributor

@chqrlie The js_typedarray_type_t enum is ABI compatible with napi_typedarray_type to make conversion a simple cast: https://github.com/nodejs/node/blob/8e6a45b3e5763bb065e4aa6f31f20e5b7cb58760/src/js_native_api_types.h#L94-L106

@chqrlie
Copy link
Collaborator

chqrlie commented May 10, 2024

@chqrlie The js_typedarray_type_t enum is ABI compatible with napi_typedarray_type to make conversion a simple cast:

OK node has the enum, not v8... We should definitely renumber our own enum to make it compatible.

@kasperisager
Copy link
Contributor

Yep, for V8 we have to fetch the corresponding constructor function, it doesn't have a type enum: https://github.com/holepunchto/libjs/blob/41ba260766291ca2477a1473c652d5a5494e3f7d/src/js.cc#L3974-L4001

@xeioex
Copy link
Contributor Author

xeioex commented May 10, 2024

@chqrlie

Regarding typed arrays, the APIs should either create a view on an existing ArrayBuffer / SharedArrayBuffer or create the full view of an ArrayBuffer implicitly created from raw data. I am not sure what is the best way to approach this.

I agree, that is why the initial API was argc, argv.

The function behaves the same as the various typed array constructors in that it takes an arraybuffer, an offset, and a length and constructs a view over that.

How create a view on an existing ArrayBuffer / SharedArrayBuffer C prototype should look like?

What about:
int JS_NewTypedArrayFromBuffer(JSContext *ctx, JSTypedArrayEnum type, JSValue arrayBuffer, size_t offset, size_t length)?

I am not sure about what is the best arrayBuffer type.

@xeioex
Copy link
Contributor Author

xeioex commented May 10, 2024

but the one I use the most is the one that doesn't copy.

Zero-copy version sounds good too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants