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

WIP using JSI's NativeState class for faster access to native C++ object when Hermes enabled #4871

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tomduncalf
Copy link
Contributor

What, How & Why?

This closes # ???

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📦 Updated internal package version in consuming package.jsons (if updating internal packages)
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@cla-bot cla-bot bot added the cla: yes label Sep 5, 2022
@tomduncalf
Copy link
Contributor Author

tomduncalf commented Sep 5, 2022

@kraenhansen @RedBeard0531 I wanted to open this WIP to start a discussion around supporting the new NativeState API in Hermes master, which provides a way to store a C++ object on a JSI object and allows us to have much faster access to the native object when Hermes is enabled. It is not currently in an RN release, so the build will fail (you need to build RN from source to make it work).

It seems to work (just running the performance tests) but I am sure I'll have missed something as this stuff is complex!

The main gotcha here is that the NativeState API is not implemented for the JSCRuntime and instead throws a logic_error, so I am catching that and falling back to the old property method, as I couldn't find a nicer way to detect if Hermes is enabled.

Another gotcha is this will tie us to a minimum of whatever RN version includes this API. Not sure if that is an issue or if there's a good way to work around that if so.

@tomduncalf
Copy link
Contributor Author

tomduncalf commented Sep 5, 2022

OK adding the try/catch makes this like 20x slower with JSC (compared to just having the "catch" code there), lol, I guess that won't fly... maybe we could store a class static optional flag native_state_supported, then either set that to true or false depending on whether we hit a std::logic_error the first time we try to set_internal... then in future, skip the try catch and just run the appropriate block based on that flag. (Edit: see below comment for rough version)

Any better ideas @RedBeard0531?

@tomduncalf
Copy link
Contributor Author

I pushed a rough version of what I described above but I'll leave the comment intact in case the history gets confusing! 2a8cceb

@tomduncalf tomduncalf force-pushed the td/v11-rn-0.70-use-internal-state branch from 3195ee2 to 7d6db31 Compare October 24, 2022 08:57
@tomduncalf tomduncalf changed the base branch from v11 to master October 24, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant