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 methods to create ArrayBuffers from byte arrays #419

Closed
wants to merge 3 commits into from

Conversation

savv
Copy link

@savv savv commented Nov 22, 2020

Summary

This PR adds a factory method to the ArrayBuffer class, so that JSI methods can return them as results. It is a continuation of facebook/react-native#30445

Test Plan


This change is Reviewable

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 22, 2020
@savv
Copy link
Author

savv commented Nov 22, 2020

@tmikov @ryantrem FYI. Thanks for your feedback so far.

@savv
Copy link
Author

savv commented Nov 25, 2020

@dulinriley @avp
Would you have any recommendations on how to get this PR merged in?
Thanks

@tmikov tmikov requested a review from mhorowitz November 25, 2020 17:50
@tmikov
Copy link
Contributor

tmikov commented Nov 25, 2020

@savv we have a company holiday this week, but I am hoping @mhorowitz, who designed JSI, will take a look after.

@savv
Copy link
Author

savv commented Nov 26, 2020

Ah of course, thanks for the heads up and enjoy thanksgiving!

@savv
Copy link
Author

savv commented Dec 2, 2020

Hi @mhorowitz hope you enjoyed your holidays. Let me know if you have time to review this PR.

@savv
Copy link
Author

savv commented Dec 7, 2020

@ryantrem @tmikov @mhorowitz would you have any advice on how to proceed with this PR? Thanks!

@ryantrem
Copy link
Contributor

ryantrem commented Dec 9, 2020

Someone from Facebook needs to look, presumably @mhorowitz.

@mhorowitz
Copy link
Contributor

mhorowitz commented Dec 9, 2020

Yeah, this is on me. Sorry, it's been a busy week and I keep hoping to get to it, but I haven't yet. It is on my list! Sorry for the delay.

Copy link
Contributor

@mhorowitz mhorowitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I make any change to JSI, one question I always ask is whether or not this kind of thing requires changes to JSI. There's a cost to adding per-engine methods. In this case, the functionality you want could be done purely as JSI client code, which really simplifies things.

It appears we have a gap in our JSI tests, in that there are no ArrayBuffer tests. So I wrote one: https://gist.github.com/mhorowitz/4915d43b8d82ae0eb98a157e689ba23e

In this test, I create an ArrayBuffer, then copy contents into it from C++. I believe this is exactly what you are trying to do. The test isn't exactly what you would do in a library, but it's easy to see how you could use lines 9,10,12,14 in the gist to create a createFromBytes() method. The only tradeoff is that getting the function could be cached, but isn't here. So perhaps another approach would be to have an ArrayBuffer::getConstructor() function which returns the Function, which can be cached by the caller, or just used immediately. We could easily write both, and have the former call the latter. Either of these implementations should work across all JSI Runtime classes. I've run the test in my gist against JSCRuntime and HermesRuntime.

So, before I review this PR in more detail, what do you think? Another nice thing about this approach is that the delay to get new features from JSI into RN releases is pretty long. You could include a private implementation of createFromBytes as a local standalone function in your C++ JSI code, and include it in your library much more quickly.

Either way, I'll probably land the gist, because we should have at least some tests. I may add more.

@bghgary
Copy link

bghgary commented Dec 14, 2020

We would be in support of this kind of functionality directly in JSI. Many JS engines (JSC, V8, Chakra) support creation of an ArrayBuffer directly from a native buffer without copying. This PR by itself might not be enough as there also needs to be some way to know when the js array buffer no longer needs the native buffer (i.e. when the finalizer happens for the js array buffer) maybe via a callback. I'm assuming that Hermes doesn't support this scenario at the moment as this PR does copy. It would improve performance for large arrays to avoid copying.

@mhorowitz
Copy link
Contributor

mhorowitz commented Dec 15, 2020

Creating an ArrayBuffer without copying seems like a more reasonable use case for extending JSI. But as you point out, this PR does not create a zero-copy interface, which informed my response.

The Hermes ArrayBuffer impl does have an internal finalizer to free the buffer, but it's not something which can be set externally right now. A PR would need to add that before we could have JSI include a zero-copy interface.

@savv
Copy link
Author

savv commented Dec 16, 2020

Hi @mhorowitz Thank you for reviewing my PR and for your suggestion! I haven't had the chance to try your suggestion yet, but I can't see why it wouldn't work. And it's great for me as I don't have to wait.

Before we close this PR: I guess your approach would work to replace any factory method (e.g. createStringFromAscii), so maybe it's still valuable to add it?

The only tradeoff is that getting the function could be cached, but isn't here

Can this be done? Is there a risk that the underlying implementation gets moved? (e.g. if the user monkey patches global.ArrayBuffer, will the constructor get GC'ed?)

Using move semantics would be great, although I haven't seen those used in this codebase. For example, there doesn't seem to be createStringFrom* methods that make use of std::move or take over the passed memory?

@mhorowitz
Copy link
Contributor

mhorowitz commented Dec 17, 2020

Before we close this PR: I guess your approach would work to replace any factory method (e.g. createStringFromAscii), so maybe it's still valuable to add it?

I'm not sure I understand your question. Are you asking why there are two different createStringFromX calls? They exist separately because some engines support multiple internal string representations. If we only have a single createString method, then potential efficiency is lost. (Arguably, we could have a char16_t call, too.) createArrayBufferFromBytes() is not the same, as there is no efficiency lost by doing the copy externally to the engine. The two behaviors are essentially identical.

Can this be done? Is there a risk that the underlying implementation gets moved? (e.g. if the user monkey patches global.ArrayBuffer, will the constructor get GC'ed?)

It can be cached. If you store it in a jsi::Function, it will never be GC'd, so a monkey patch which changes it would not be a problem. On the other hand, what if you want to monkey patch it and use that version when creating an ArrayBuffer from C++? If you cache it in your own code, then you can decide on the behavior which works best for you. If we were to bake this into a library call, we'd either need to pick a strategy, making it less versatile, or design a more complex API.

Using move semantics would be great, although I haven't seen those used in this codebase. For example, there doesn't seem to be createStringFrom* methods that make use of std::move or take over the passed memory?

evaluateJavaScript and createFromHostObject take shared_ptr references, which is halfway there. createFromHostFunction takes its Function parameter by value, as described by Dave Abrahams in https://web.archive.org/web/20140205194657/http://cpp-next.com/archive/2009/08/want-speed-pass-by-value/.

We don't do this for createStringFromX because for most strings, we want to store them in the JS heap. Since JS manages its own memory, we need to do a copy to get the strings into the JS heap. In theory we could support external string data, but so far, we haven't seen cases to justify the complexity.

That said, we don't store ArrayBuffer's native backing buffer in the JS heap, and such buffers can be large and expensive to copy, so the tradeoff is arguably different here. I also don't think move/sink semantics are ideal, because native code might want to share access to the buffer, so I would use std::shared_ptr instead. As I said above, supporting this would require more changes internal to Hermes.

In case someone is interested in putting together a PR, here's the interface I think would make most sense:

class Runtime {
  virtual ArrayBuffer createArrayBufferFromBuffer(std::shared_ptr<Buffer> buffer) = 0;
};

This provides maximum flexibility for both the caller and engine implementations.

@savv
Copy link
Author

savv commented Dec 19, 2020

@mhorowitz I can confirm that I got it to work on my end, as expected! I am happy to report that using JSI in my library, https://github.com/savv/react-native-leveldb/, I can read/write keys 2-4x faster than with AsyncStorage or react-native-sqlite-storage. From my side, we could close this PR is there's no use for it. Or I could adapt it to use shared_ptr, if somebody could help me to adapt HermesRuntimeImpl::arrayBufferHVFromBytes.

I didn't manage to cache the function as you suggested; I got a cryptic exception exception in method JSCRuntime::callAsConstructor. Not caching the constructor function fixed this (greentriangle/react-native-leveldb@2ccdf5d).
image

I'm not sure I understand your question. Are you asking why there are two different createStringFromX calls?

I meant that the same pattern (obtaining the constructor function and calling it) could be used to initialize strings or any other type.

@mhorowitz
Copy link
Contributor

@savv Glad you got it working, and those perf numbers sound great.

I can't tell from what you posted there why caching didn't work correctly for you. I'd need to see the code you were using to do the caching.

I'll close this for now, since you've got a solution. If someone wants to work on a PR, we can open a new issue, or reopen this one.

I meant that the same pattern (obtaining the constructor function and calling it) could be used to initialize strings or any other type.

That won't work. If you obtain the String ctor... what do you pass to it to create a string from C++ data? Unlike ArrayBuffers, strings are immutable, so you can't create one then modify it after the fact. So, JSI has to provide a mechanism to create strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants