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

Allow to access typed arrays by reference instead of copy #47

Closed
letmaik opened this issue Oct 7, 2020 · 11 comments · Fixed by #48
Closed

Allow to access typed arrays by reference instead of copy #47

letmaik opened this issue Oct 7, 2020 · 11 comments · Fixed by #48
Assignees
Labels
enhancement New feature or request

Comments

@letmaik
Copy link

letmaik commented Oct 7, 2020

Returning a typed array from AS to JS seems to make a copy of it. However, I'd like to obtain a view on the same underlying ArrayBuffer. Looks like .slice() is responsible for the copy:

return wasmExports.__getInt8Array(responseRef).slice();

@torch2424 torch2424 self-assigned this Oct 7, 2020
@torch2424 torch2424 added the enhancement New feature or request label Oct 7, 2020
@torch2424
Copy link
Owner

Ah! Thanks for bringing this up. Yeah currently we are copying because of the context in: #9

But looking at AssemblyScript/assemblyscript#1047. I'm not too sure if this would introduce breaking changes.

cc @MaxGraey , do you happen to know if the removing these .slice() calls would be safe to use __getXXArrayView? 🤔

@torch2424
Copy link
Owner

torch2424 commented Oct 7, 2020

Also, I don't want to slow you down @letmaik , if your comfortable, AsBind essentially wraps around the AS loader, and makes it more convenient.

I don't know what project you are working on (would be super interested to hear if that's okay! No worries if not! 😄 ), but if you don't mind getting a bit more lower level with passing types around, feel free to check that out as well because then you can choose between copying vs getting a direct view 😄

@MaxGraey
Copy link

MaxGraey commented Oct 7, 2020

__getXXArrayView just create view representation to some region of linear memory based on pointer which comes from AS. So it's really cheap and don't copy any data, but it's also unsafe due to array buffer (memory.buffer) may detached after any other call from assemblyscript side which use heap, so much better use owned copy of this view. That's what do __getXXArray functions. Its just the same as __getXXArrayView(...).slice(). You could use __getXXArrayView but you should totally understand what happening on AS side during lifetime of __getXXArrayView instance.

@MaxGraey
Copy link

MaxGraey commented Oct 7, 2020

wasmExports.__getInt8Array(responseRef).slice(); 

This redundant. It should be just wasmExports.__getInt8Array(responseRef) or wasmExports.__getInt8ArrayView(responseRef).slice() which is basically the same.

@letmaik
Copy link
Author

letmaik commented Oct 8, 2020

I don't know what project you are working on (would be super interested to hear if that's okay! No worries if not! 😄 ), but if you don't mind getting a bit more lower level with passing types around, feel free to check that out as well because then you can choose between copying vs getting a direct view 😄

This is for a hobby project, basically my first emulator (for a simple system to start with, CHIP-8), and I'd like to use AS because it seems really nice :) So there's definitely no urgency here but I'd like to do it the proper way as if it was a performance-critical piece of code, while also staying as high-level as possible. That's why I'm looking at as-bind to remove the nasty details at the boundary.

__getXXArrayView just create view representation to some region of linear memory based on pointer which comes from AS. So it's really cheap and don't copy any data, but it's also unsafe due to array buffer (memory.buffer) may detached after any other call from assemblyscript side which use heap [...]

I'd like to understand this a bit better. As long as the typed array object exists in wasm memory, it is safe to use it in JS, right? Or is there a case where an existing object can get relocated to a different location in linear memory?

@MaxGraey
Copy link

MaxGraey commented Oct 8, 2020

As long as the typed array object exists in wasm memory, it is safe to use it in JS, right?

using __getXXArrayView for long lifetime defenitely unsafe, but if you have something like this:

const arrPtr = wasm.someProcessing();
let typedArray = wasm.__getUint8ArrayView(arrPtr);

pureJSProcessing(typedArray);  // don't call any methods from wasm. Just JS routine

wasm.__release(arrPtr); // free resource

It could work properly.

But if pureJSProcessing for example call some other or the same wasm method which could potentially alloc or dealloc memory in heap it may cause to detouched arraybuffer or invalid data associated to this arraybuffer when you try get access to its elements afterwards.

I hope this is clearer now =)

@letmaik
Copy link
Author

letmaik commented Oct 8, 2020

I found a description of the same issue here: emscripten-core/emscripten#6747

I think as-bind should allow to get an unsafe view on array buffers. This could be opt-in by setting some config option similar to the type caching option:

asBindInstance.exports.myFunction.returnUnsafeArrayViews = true; // default false
// or globally:
asBindInstance.returnUnsafeArrayViews = true;

The docs should mention that by default a copy is returned, but advanced users can opt in to get a view instead, together with all the caveats, e.g. the array buffer must still be alive/referenced inside wasm (since the reference count will not be increased when returning the buffer), and no further wasm function should be invoked (to avoid triggering memory growth which would detach the view).

@torch2424
Copy link
Owner

@MaxGraey

This redundant. It should be just wasmExports.__getInt8Array(responseRef) or wasmExports.__getInt8ArrayView(responseRef).slice() which is basically the same.

Ah for sure! Thanks for catching that! Yeah I remember we used to need it for some reason 🤔

using __getXXArrayView for long lifetime defenitely unsafe

Thank you for clarifying! Yeah I remember bumping into this a while back, but then we fixed it in the loader 😄


@letmaik

This is for a hobby project, basically my first emulator

Oh nice! Yeah emulators are fun! That's super awesome! 😄 🎉

I think as-bind should allow to get an unsafe view on array buffers. This could be opt-in by setting some config option similar to the type caching option:

Yeah, so we can totally add this option on functions, I think it'd work! Though, I'll probably think of a different name. Probably like unsafeReturnValue. Since, in the future maybe we will have a getStringUnsafe, or getClassUnsafe etc... in the loader. Also, if we can ever pass an unsafe parameter (unlikely), we could name it unsafeParams or something 😄

The change mostly needs to be done here: https://github.com/torch2424/as-bind/blob/master/lib/asbind-instance/supported-ref-types.js#L127

Providing an getUnsafeValueFromRef. And then upadating this block: https://github.com/torch2424/as-bind/blob/master/lib/asbind-instance/bind-function.js#L229

Actually, that's super easy. I'll just do that now 😂 GIve me like 10 minutes 😄

@torch2424
Copy link
Owner

@letmaik

Should be live in 0.4.1 😄 Docs are there on the README on how to use it 😄 🎉

@letmaik
Copy link
Author

letmaik commented Oct 10, 2020

@torch2424 Great! Just out of curiosity, why does it return the ptr and the array? What can you do with the ptr?

@torch2424
Copy link
Owner

torch2424 commented Oct 11, 2020

@letmaik Yay! Glad it's working for you!

I returned them both for a few reasons:

  1. So it is more obvious that it's returning the unsafe version. I figured it'd be a lot easier to debug if the flag got set somewhere by accident, and then someone else calls the function, expecting it to copy.

  2. Since you have bi-directional view into Wasm memory, if you want to modify the array, it'd maybe be helpful to know where the array is, or you could wrap in a different typed array, rather than having to write another function in Wasm.

  3. Itd be alot easier to test this for both me, and consumers of the API 😀

I was hoenstly a bit 50/50 if it should return the pointer. But I figured more good than harm could come from it 😀

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

Successfully merging a pull request may close this issue.

3 participants