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

support passing ArrayBuffer to wasm module #1961

Open
art-in opened this issue Jan 16, 2020 · 6 comments
Open

support passing ArrayBuffer to wasm module #1961

art-in opened this issue Jan 16, 2020 · 6 comments
Labels

Comments

@art-in
Copy link

art-in commented Jan 16, 2020

i'm passing ArrayBuffer from browser window to worker thread, and then to wasm module function compiled with rust and wasm-pack, but rust always receives buffer of zero length.

rust function:

#[wasm_bindgen]
pub fn process_array_buffer(buffer: &[u8]) -> i32 {
    buffer.len() as i32
}

generated glue js:

function passArray8ToWasm0(arg, malloc) {
    const ptr = malloc(arg.length * 1);
    getUint8Memory0().set(arg, ptr / 1);
    WASM_VECTOR_LEN = arg.length;
    return ptr;
}
/**
* @param {Uint8Array} buffer
* @returns {number}
*/
export function process_array_buffer(buffer) {
    var ptr0 = passArray8ToWasm0(buffer, wasm.__wbindgen_malloc);
    var len0 = WASM_VECTOR_LEN;
    var ret = wasm.process_array_buffer(ptr0, len0);
    return ret;
}

worker.js:

if (e.data instanceof ArrayBuffer) {
  const res = wasm.process_array_buffer(e.data);
  console.log('worker: res', res); // returns 0
}

so glue js expects typed array Uint8Array, but i'm passing ArrayBuffer, which doesn't have .length prop but .byteLength, etc.

i'm working with ArrayBuffer because it's Transferable, and Uint8Array is not.

as a workaround i can receive ArrayBuffer in worker, wrap it into Uint8Array there and pass to wasm.

but i believe glue js can check type of argument with instanceof ArrayBuffer, and pass it to wasm memory correctly.

@alexcrichton
Copy link
Contributor

Thanks for the report! This is indeed not supported today, but seems plausible to me to support for u8 slices specifically

@Pauan
Copy link
Contributor

Pauan commented Jan 16, 2020

I don't think we should be using instanceof, that would impose a runtime cost on everyone, even those who are correctly passing in Uint8Array. Rust's philosophy is that you should only pay a performance cost for the features you actually use.

Because ArrayBuffer is an abstract blob of bits without any structure, it is necessary (even in pure JS) to wrap it in a typed array like Uint8Array, so that way the byte size is known. So manually wrapping it in Uint8Array is the correct approach:

if (e.data instanceof ArrayBuffer) {
  const res = wasm.process_array_buffer(new Uint8Array(e.data));
  console.log('worker: res', res);
}

It's important to note that using new Uint8Array on an ArrayBuffer does not copy the bytes, it simply creates a live view. So this is extremely fast.

@art-in
Copy link
Author

art-in commented Jan 16, 2020

that's right "only pay for what you use" philosophy works great inside c++/rust where we have strong type system. but here in js it just receives undefined for arrayBuffer.length, because it should be arrayBuffer.byteLength

so currently passing ArrayBuffer fails silently (!)

besides, there's type checking for numeric arguments already

function _assertNum(n) {
    if (typeof(n) !== 'number') throw new Error('expected a number argument');
}

maybe not implicit conversion, but type assertion should exist at least in debug build

@Pauan
Copy link
Contributor

Pauan commented Jan 16, 2020

I completely agree, we should have runtime assertions in debug mode (not release mode). Silent errors are quite bad.

@DougAnderson444
Copy link
Contributor

Ran across this silent error today.

@daxpedda daxpedda added bug and removed question labels Mar 1, 2024
@daxpedda
Copy link
Collaborator

daxpedda commented Mar 1, 2024

I consider this a bug, happy to review a PR.

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

No branches or pull requests

5 participants