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

[TM][C++] Improve C++ TurboModule system #28128

Open
RSNara opened this issue Feb 19, 2020 · 12 comments
Open

[TM][C++] Improve C++ TurboModule system #28128

RSNara opened this issue Feb 19, 2020 · 12 comments
Assignees
Labels
Help Wanted :octocat: Issues ideal for external contributors. RN Team

Comments

@RSNara
Copy link
Contributor

RSNara commented Feb 19, 2020

Context

The C++ TurboModule system, unlike the ObjC and Java systems, is fairly immature. It has a lot of problems that need to be addressed, and gaps that need to be filled. Unfortunately, at Facebook, we only have a handful of C++ NativeModules. Additionally, there's no urgency to convert these NativeModules to C++-only TurboModules, because these NativeModules are already going through a bridging layer in the TurboModule system (i.e: we could turn off the old NativeModule system and still have these NativeModules fully functional). Therefore, it's difficult to prioritize development of the C++ TurboModule system internally.

For transparency, this is my focus in H1 2020:

  • Open source and internal iOS NativeModule migration.
  • Internal rollout of the TurboModule system (on both iOS and Android).
  • The effort to make TurboModules and Codegen open source ready.

With all the things on my plate this half, I find it unlikely that the C++ TurboModule system will be finished any time soon. So, this GitHub issue documents the work that's required to build out the C++ TurboModule system. If anyone is interested in taking ownership over this portion of the TurboModule project, please feel free to comment below. I'd love to work together and see this completed. 😁

Problem

This is currently the interface of our C++-only TurboModules:

class SampleTurboCxxModule : public NativeSampleTurboCxxModuleSpecJSI {
 public:
  SampleTurboCxxModule(std::shared_ptr<CallInvoker> jsInvoker);

  void voidFunc(jsi::Runtime &rt) override;
  bool getBool(jsi::Runtime &rt, bool arg) override;
  double getNumber(jsi::Runtime &rt, double arg) override;
  jsi::String getString(jsi::Runtime &rt, const jsi::String &arg) override;
  jsi::Array getArray(jsi::Runtime &rt, const jsi::Array &arg) override;
  jsi::Object getObject(jsi::Runtime &rt, const jsi::Object &arg) override;
  jsi::Object getValue(
      jsi::Runtime &rt,
      double x,
      const jsi::String &y,
      const jsi::Object &z) override;
  void getValueWithCallback(jsi::Runtime &rt, const jsi::Function &callback)
      override;
  jsi::Value getValueWithPromise(jsi::Runtime &rt, bool error) override;
  jsi::Object getConstants(jsi::Runtime &rt) override;
};

There are a few problems with this API.

  1. Hermes is a "bring your own locks" VM. The application/framework using Hermes is responsible for making sure that jsi::Runtime is accessed safely. This simply isn't possible if we provide all C++-only TurboModules with access to the jsi::Runtime.
  2. It's not a good idea to give C++-only TurboModules ownership of JSI objects (eg:jsi::Function, jsi::String, jsi::Array, jsi::Object). First, this isn't a very clean API. Second, by design, many of these JSI objects (jsi::Function especially) can't outlive the jsi::Runtime (see the jsi.h). So, we should had C++-only TurboModules safe wrappers around these objects, so that the TurboModule framework can manage their lifecycles.

Other things to think about:

  1. How do we perform cleanup for C++-only TurboModules? Should we expect C++-only TurboModules to have a custom invalidate method, or is using their destructor fine?
  2. There is no CxxTurboModule class, like we have JavaTurboModule, and ObjCTurboModule. If we want to make C++-only TurboModules more robust without bloating the codegen, this class would be necessary.

How do I start?

There is currently only one pure C++ TurboModule: SampleTurboCxxModule. This TurboModule extends its "code-generated" spec NativeSampleTurboCxxModuleSpecJSI (the spec is really just hand-written). This spec directly extends TurboModule, as you can see here.

  1. Create a new CxxTurboModule class and have it extend TurboModule. Have NativeSampleTurboCxxModuleSpecJSI extend CxxTurboModule.
  2. You can call into CxxTurboModule from the "code-generated" __hostFunctions in NativeSampleTurboCxxModuleSpecJSI. Feel free to modify the contents of the __hostFunctions in the codegen. Please look at RCTSampleTurboModuleSpec.mm for an example of how we do this in iOS. Some of the responsibilities of CxxTurboModule:
    1. Convert JS arguments (JSI objects) to vanilla C++ objects.
      1. What do we convert the Object type to? Should we use folly::dynamic?
      2. What do we convert object literals (eg: {| foo: bar |}) to? In ObjC, we convert them to structs, since we know the type of each property.
      3. What do we convert Array<T> to? Should we just use an STL container?
    2. Convert C++ returns to JSI objects.
      1. How do we handle Promise returns?
      2. Do we simply pass in the C++-only TurboModule method a resolve and a reject lambda, like in ObjC, or do we pass in a "Promise" object, like we do in Android. Alternatively, should we leverage some STL data structure (std::promise)?
    3. Dispatch async method calls appropriately on each platform. This will require the CxxTurboModule constructor to accept a native CallInvoker that dispatches work to a background thread.
      • On iOS, every C++ NativeModule calls its async methods on its own ObjC method queue.
        1. MethodQueue is created here, for each NativeModule: code.
        2. C++ NativeModule list is created here, from RCTModuleData objects: code.
        3. DispatchMessageQueueThread is the class our C++ NativeModules use to dispatch to the MessageQueue. Here's the async dispatch method: code.
      • On Android, all NativeModule async methods dispatch to the same NativeModules thread.
        1. The NativeModules thread is created by the bridge here: code.
        2. It's passed into buildNativeModuleList here: code.
        3. And used to create CxxNativeModule (i.e: legacy Cxx Native Modules) here: code.
    4. Asynchronously invoke JS callbacks (ex: jsi::Functions) on the JS Thread. This will require the CxxTurboModule constructor to accept a CallInvoker that dispatches work to the JS thread. Also, we'll have to store all the jsi::Function objects passed from JS to C++ so that they can be invoked later by the JS thread. This presents an additional problem:
      • When the TurboModuleManager is destroyed, ensure that all held jsi::Functions (i.e: JS callbacks that were passed into C++ from JS) are destroyed. In iOS and Android, we accomplish this by using LongLivedObjectCollection, and CallbackWrapper. LongLivedObjectCollection is cleared when the runtime is destroyed.

How do I test?

To test your changes, in RNTester on iOS, access the TurboModule in JS via TurboModuleRegistry.get('SampleTurboCxxModule').

@RSNara RSNara self-assigned this Feb 19, 2020
@RSNara RSNara changed the title [TM][C++] Improve C++ TurboModules [TM][C++] Improve C++- TurboModule system Feb 19, 2020
@RSNara RSNara changed the title [TM][C++] Improve C++- TurboModule system [TM][C++] Improve C++ TurboModule system Feb 19, 2020
@ejanzer
Copy link

ejanzer commented Feb 19, 2020

Adding some of the people at MSFT we've discussed this with: @stmoy, @acoates-ms

@react-native-bot

This comment has been minimized.

@react-native-bot react-native-bot added the Needs: Environment Info Please run `react-native info` and edit your issue with that command's output. label Feb 19, 2020
@hramos hramos added Good first issue Interested in collaborating? Take a stab at fixing one of these issues. RN Team Help Wanted :octocat: Issues ideal for external contributors. and removed Needs: Environment Info Please run `react-native info` and edit your issue with that command's output. Needs: Triage 🔍 Good first issue Interested in collaborating? Take a stab at fixing one of these issues. labels Feb 20, 2020
@KrisSiegel
Copy link

Curious, it looks like there is a need to abstract away from the jsi::* objects to have something that's more universal / could theoretically work against more than just Hermes but at the same time much of the todo list seems to simply the continued used of jsi::* objects.

Is the intent here to create a more generic way to create modules that do not care about the underlying JS engine? As that is something I am also interested in doing :)

@RSNara
Copy link
Contributor Author

RSNara commented Mar 6, 2020

Curious, it looks like there is a need to abstract away from the jsi::* objects to have something that's more universal / could theoretically work against more than just Hermes but at the same time much of the todo list seems to simply the continued used of jsi::* objects.

Your C++ TurboModule shouldn't have to know about the JS VM. But this doesn't mean that we stop using jsi::* objects. We need them to expose the TurboModule to JS. @KrisSiegel , does that answer your question?

@asklar
Copy link
Contributor

asklar commented Oct 1, 2020

it looks like TurboModules are currently coupled to STL types which makes the APIs non ABI-safe, thus hindering reuse-via-binary, are there plans to address this?

@sintylapse
Copy link

sintylapse commented Oct 5, 2020

This issue needs to be prioritized for realm/realm-js#2455 . It's a huge blocker

@8BallBomBom
Copy link

@shergin @mdvacca @JoshuaGross @hramos @fkgozali
Sorry for the random ping.

Any chance of seeing any progress on this issue?
It is apparently preventing RealmJS updating their module to get Hermes support going.
realm/realm-js#2455 (comment)

@wooklym
Copy link

wooklym commented Apr 27, 2021

@rush86999
Copy link

+1 support for realmjs with turbomodule

@mrousavy
Copy link
Contributor

I know this is an old issue and it seems like C++ TurboModules have matured since then (nice work!), but I've been struggling a bit with the setup.

I'm making react-native-mmkv a C++ TurboModule (feat/turbomodule branch) as it only has one line of platform specific code, but I stumbled upon a few things:

  1. On Android, autolinking just got supported in RN 0.74 and latest RN CLI by Nicola 👏
  2. On iOS, autolinking is not currently supported, so this is a blocker for me. :(
  3. On Android, I can't seem to correctly setup my CMakeLists.txt. The one provided by the example and the documentation don't build for me
In file included from /Users/mrousavy/Projects/react-native-mmkv/android/build/generated/source/codegen/jni/RNMmkvSpec-generated.cpp:11:
/Users/mrousavy/Projects/react-native-mmkv/android/build/generated/source/codegen/jni/./RNMmkvSpec.h:13:10: fatal error: 'ReactCommon/JavaTurboModule.h' file not found
#include <ReactCommon/JavaTurboModule.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Is there maybe a different example C++ TurboModule that I can take a look at?

@cortinico
Copy link
Contributor

I know this is an old issue and it seems like C++ TurboModules have matured since then (nice work!), but I've been struggling a bit with the setup.

I'm making react-native-mmkv a C++ TurboModule (feat/turbomodule branch) as it only has one line of platform specific code, but I stumbled upon a few things:

  1. On Android, autolinking just got supported in RN 0.74 and latest RN CLI by Nicola 👏
  2. On iOS, autolinking is not currently supported, so this is a blocker for me. :(
  3. On Android, I can't seem to correctly setup my CMakeLists.txt. The one provided by the example and the documentation don't build for me
In file included from /Users/mrousavy/Projects/react-native-mmkv/android/build/generated/source/codegen/jni/RNMmkvSpec-generated.cpp:11:
/Users/mrousavy/Projects/react-native-mmkv/android/build/generated/source/codegen/jni/./RNMmkvSpec.h:13:10: fatal error: 'ReactCommon/JavaTurboModule.h' file not found
#include <ReactCommon/JavaTurboModule.h>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Is there maybe a different example C++ TurboModule that I can take a look at?

For future reference, an example of modules with C++ Autolinking is here: https://github.com/cortinico/reproducer-cxx-tm-autolinking/

@mrousavy
Copy link
Contributor

If anyone encounters the same error as me make sure that you remove externalNativeBuild from your library's build.gradle file.

After all, the RN CodeGen project should build your C++ project, not your build.gradle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted :octocat: Issues ideal for external contributors. RN Team
Projects
None yet
Development

No branches or pull requests