Skip to content

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

Closed
wants to merge 4 commits into from

Conversation

ron99
Copy link

@ron99 ron99 commented Oct 11, 2017

I added efficient implementation to the vecFromJSArray function for numeric types. The function checks if the requested vector type has a matching TypedArray and if so, uses it to copy the array / array-like object directly into the Heap buffer 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 using val::as function to convert the JS value to c++ value.

Fixes #5519.

@kripken kripken added the embind label Oct 11, 2017
}

val memory = val::module_property("buffer");
val memoryView = val::global(arrayType).new_(memory, reinterpret_cast<std::uintptr_t>(rv.data()), l);
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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.

arrayType = "Float64Array";
break;
default:
throw;
Copy link
Collaborator

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?

Copy link
Author

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.

@bvibber
Copy link
Collaborator

bvibber commented Apr 29, 2018

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 throw for the unreachable logic error or will that cause pulling in exception stuff when it's not actually used?

@blastrock
Copy link

It would be nice to merge this

@miwelc
Copy link
Contributor

miwelc commented Mar 6, 2019

Any news on this?

@robfors
Copy link
Contributor

robfors commented Mar 28, 2019

I would like to see this feature added as well.
However, 936efed seems to break it. That commit removes the needed Module.buffer.
When I send it a Uint8Array of length 413 I get a Uncaught RangeError: Source is too large in Chromium and a similar error message in Firefox at the line memoryView.call<void>("set", v);.

@svenpilz
Copy link

When I send it a Uint8Array of length 413 I get a Uncaught RangeError: Source is too large in Chromium and a similar error message in Firefox at the line memoryView.call<void>("set", v);.

memoryView["length"].as<unsigned>() is now 0, where it had the size given in new_ before. Also for other types, not only Uint8.

@svenpilz
Copy link

svenpilz commented Aug 12, 2019

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 vecFromJSArray function:

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.

@sbc100 sbc100 closed this Jan 30, 2020
@kripken
Copy link
Member

kripken commented Jan 31, 2020

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.

cc @jgravelle-google

kripken added a commit that referenced this pull request Oct 7, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants