-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Efficient conversion between JS arrays and c++ vectors for numeric types #5655
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
Conversation
} | ||
|
||
val memory = val::module_property("buffer"); | ||
val memoryView = val::global(arrayType).new_(memory, reinterpret_cast<std::uintptr_t>(rv.data()), l); |
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.
Where does the deletion of this new_
occur?
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.
new_
creates a JS TypedArray
object, which cannot be explicitly deleted, but eventually will be collected by the JS engine Garbage Collector.
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.
Ah right, val
is refcounted, and deleted once references drop to zero. I was thinking about the Embind objects that need to be manually .delete()
d here.
system/include/emscripten/val.h
Outdated
arrayType = "Float64Array"; | ||
break; | ||
default: | ||
throw; |
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.
Hmm, I thought that throw;
is only allowed inside a catch()
handler to rethrow the exception object. What does a throw;
statement outside any catch()
handlers throw? Does this compile?
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 syntax (used outside a catch
block) actually terminates the program. I added this because it should never be reached (due to the typeSupportsMemoryView
check before).
I'll replace it with an explaining comment and throw a logic_error
.
Looks workable (haven't tested, but this is equivalent to the optimized memcpy in the runtime, roughly). Quick question -- is it ok to use a |
It would be nice to merge this |
Any news on this? |
I would like to see this feature added as well. |
|
The fix may be simple. We could replace emscripten::val memory = emscripten::val::module_property("buffer");
emscripten::val memoryView =
v["constructor"].new_(memory, reinterpret_cast<std::uintptr_t>(rv.data()), l); with emscripten::val memoryView{emscripten::typed_memory_view(l, rv.data())}; Here the complete template <typename T> std::vector<T> vecFromJSArray(const emscripten::val &v)
{
std::vector<T> rv;
const auto l = v["length"].as<unsigned>();
rv.resize(l);
emscripten::val memoryView{emscripten::typed_memory_view(l, rv.data())};
memoryView.call<void>("set", v);
return rv;
} I tested this with 1.38.31, where the previous way was no longer working. Update: Also works with 1.38.42. |
I think this was closed by mistake when we deleted the incoming branch (after switching to master), sorry about that. If this PR is still relevant please reopen and retarget to master. |
This is a followup of #5519 and #5655 since they were closed. It is possible to implement emscripten::vecFromJSArray more efficiently for numeric arrays by using the optimized TypedArray.prototype.set function. The main issue with this method is that it will silently fail(or succeed) if elements of the array or not numbers, as it does not do any type checking but instead works as if it called the javascript Number() function for each element. (See ToNumber for more details) So instead of simply updating vecFromJSArray to use this new implementation and break code (since there's no typechecking anymore) I added a new convertJSArrayToNumberVector (name subject to change) and improved performance a tiny bit for vecFromJSArray by: * Taking the val parameter by const reference instead of copy * Reserving the storage of the vector
I added efficient implementation to the
vecFromJSArray
function for numeric types. The function checks if the requested vector type has a matchingTypedArray
and if so, uses it to copy the array / array-like object directly into the Heapbuffer
and uses this memory as the vector backend.This is useful for converting large numeric arrays as it faster in orders of magnitude.
There may be some compatibility issues when the array contains non-numeric values, as any such value would be converted to a numeric value by the JS engine (
NaN
would become zero), which I don't think is the current behavior caused by usingval::as
function to convert the JS value to c++ value.Fixes #5519.