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

Add TypedArray support to jsi #182

Closed
sercand opened this issue Feb 6, 2020 · 10 comments
Closed

Add TypedArray support to jsi #182

sercand opened this issue Feb 6, 2020 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@sercand
Copy link

sercand commented Feb 6, 2020

JSI does not have the typed array interface. As a result, we have to use ArrayBuffer to pass UInt8Array data to native. Without a TypedArray interface, we can't allocate array buffer once and reuse again on typed arrays for better performance.

iOS 10+ Javascript Core and Hermes supports Typed Arrays and I believe it is plausible to add TypedArray to the JSI.

I am opening this issue here as suggested by @TheSavior in his comment.

@tmikov tmikov added enhancement New feature or request and removed enhancement New feature or request labels Feb 6, 2020
@tmikov
Copy link
Contributor

tmikov commented Feb 6, 2020

With JSI you can create a TypedArray, access its elements and get a native pointer to its contents. What more functionality is needed that is missing?

@tmikov tmikov added the need more info Awating additional info before proceeding label Feb 6, 2020
@sercand
Copy link
Author

sercand commented Feb 6, 2020

@tmikov
First, the most important thing is TypedArray currently disabled in ios JSCRuntime. One has to use their own forked JSCRuntime.cpp to use TypedArrays at iOS.

What I want to do is basicly with an example:

// allocate buffer once
const buffer = new ArrayBuffer(1024 * 64); // 64 kb buffer
// reuse buffer n times for a task
for (let i = 0; i < n; i++) {
  const length = requiredSize(i);
  // if buffer don't have enough size reallocate
  let data = new Uint8Array(buffer, offset, length); 
  // write to data
  sendDataToNative(data);
}

but jsi only has very simple Object::getArrayBuffer, ArrayBuffer::length, ArrayBuffer::data
functions for Buffers. But javascript core has really straightforward typed array api.

We have to access required fields with Object::getProperty even for simple things like getting typed array storage type (which is done by checking its string "name" property, if I know correctly). It should be as easy as in JSCore because in some cases TypedArrays becomes hot path in workload.

@tmikov
Copy link
Contributor

tmikov commented Feb 7, 2020

  • About JSCRuntime missing TypedArray support. Can you submit a PR fixing it to ReactNative? I believe this is an oversight, since the old version of JSC isn't used anymore.
  • About checking if something is a TypedArray: can't you use jsi::Runtime::instanceof()?

Lastly, if I understand you correctly, the problem isn't that JSI doesn't support TypedArray, but that it doesn't support it efficiently enough, and using the JSI API with TypedArray is not very ergonomic.

Fair enough. For the performance part it would really help if we could look at some numbers. A piece of code where accessing TypedfArray via JSI is a performance bottleneck.

About the ergonomic part of the API. What would an improved JSI API look like?

@wkozyra95
Copy link

Hi

I would also be interested in adding TypedArray support to jsi.

I believe this is an oversight, since the old version of JSC isn't used anymore.

JSC without TypedArray api was also present on iOS 9, and support for that was dropped recently facebook/react-native@829a223

Fair enough. For the performance part, it would really help if we could look at some numbers. A piece of code where accessing TypedfArray via JSI is a performance bottleneck.

I don't have actual numbers, but our implementation of webgl would be a good example (https://github.com/expo/expo/tree/master/packages/expo-gl-cpp). Performance problems will be most visible for calls that use small TypedArrays, in webgl there is a lot of that(usually called per animation frame) and some of those functions can accept various TypedArray types which will require a lot of instanceof checks.

What would an improved JSI API look like?

This is the implementation we are currently planning to use (for [email protected]) expo@48c16c7 if you would be interested I would gladly cleanup code and create PR

@wkozyra95
Copy link

Hi,
@tmikov I would be grateful for any response to my last comment.

@tmikov
Copy link
Contributor

tmikov commented Mar 31, 2020

@wkozyra95 sorry, somehow I missed this message. It looks fine to me. @mhorowitz do you think they should move ahead with the PR?

@mhorowitz
Copy link
Contributor

I have commented on the PR.

@mhorowitz mhorowitz added enhancement New feature or request and removed need more info Awating additional info before proceeding labels Jun 20, 2020
@mhorowitz
Copy link
Contributor

In #248 we concluded that there wasn't a justification to modify JSI based on efficiency. But, it is still cumbersome to use typed arrays with JSI. We would still welcome a PR to make typed arrays easier to use.

@wkozyra95
Copy link

I assumed that you meant creating totally separate library that depends on jsi.

We would still welcome a PR to make typed arrays easier to use.

Ok, I'll prepare new PR. Are you ok with the API (general structure) I proposed in original PR or do you have any suggestions?

@mhorowitz
Copy link
Contributor

I'm closing this since it's been open for several months.
There was a discussion of using JSI with typed arrays in #419, which we also decided not to move forward with.

In that thread, I included some discussion of what kind of API I think would make sense to add to support zero-copy typed array buffer sharing with native code. If someone is interesting in building this, please submit a new PR, or open a new issue to discuss your plans. Thanks!

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.

4 participants