-
Notifications
You must be signed in to change notification settings - Fork 134
RuntimeExecutor #196
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
Comments
Adding the people who have contributed to this proposal/discussion at FB: @shergin, @RSNara, @mhorowitz @fkgozali And some people who might be interested: @vmoroz @acoates-ms This proposal isn't something we're completely committed to, and we still have some details to iron out, but I wanted to open this up for discussion, especially with the maintainers of other platforms. |
Talking about the In general, no standard C++ standard classes are ABI safe except for some trivial cases. The industry usually uses:
E.g. the Chakra JS engine API is C-based and thus ABI safe. Another alternative for ABI-safe APIs is to use v-tables. A v-table is essentially an array with function pointers. Most compilers use the same predefined v-table layout to keep compatibility with Microsoft's COM to be able to use Windows API. Thus, in addition to C-style APIs we can define ABI-safe API based on C++ abstract virtual interfaces. We can either make them COM-compatible and derive all interfaces from In that case instead of |
@vmoroz As a person who wrote that sentence, I absolutely agree with you. Personally, I don't think we should aim for ABI stability in our C++ code at all. If some customers need ABI stability, I think we/they have to have plain C wrapper for that. Yeah, that's a controversial opinion that divided the whole C++ community into two camps. From what I see, Google (via Titus Winters) pushes against the stability. I am not aware of an official Facebook position but all I see inside (CI, repos, build configurations, culture) clearly indicates that Facebook on the same page with Google. Does Microsoft value ABI stability? (For anyone who interested in the topic, I would recommend this talk to get some context about how challenging it is to preserve the ABI stability (from |
I think I would change the sentence
to
The reason is that the implementation details should do whatever it believes the most efficient way to do for the particular moment (considering the current thread, load and so on) whereas the caller should express the desired (sync/async/sync-same-thread/async-specified-thread/and-so-on) behavior via additional helper functions (see the appendix). |
I think the more specific we make the definition of RuntimeExecutor, the more useful it becomes as an abstraction. Something that is maybe synchronous, and maybe asynchronous is dangerous. Something that is maybe asynchronous on the same thread, or maybe asynchronous on a third party thread (i.e: JS thread) is dangerous. I'm not sure how we can use RuntimeExecutor with confidence if we leave these details vague. How do I know that the same piece of code won't deadlock if we pass in a different RuntimeExecutor? Could you elaborate a bit more on why you think this is a good idea? I think my doubt stems from a lack of understanding of what you're trying to convey. Edit: Oh, I guess we won't be using RuntimeExecutor directly, but instead via these APIs that force it to be asynchronous or synchronous? But even then, why even need these utilities? |
@RSNara
What do you mean by that? When I first thought about this problem, I imagined this example:
That's absolutely understandable for any JavaScript engineer. That's why we have
I think all use-case scenarios fall into three categories:
If someone thinks that some code depends on async execution, I suspect it's already broken.
Yeah, kinda. I just expect that the vast majority of use-cases do/should not need to have any guarantees.
True. We (I) should fix it. Added:
|
I think we should replace |
tl;dr: Sorry for the rambling below. I think we should do 2.
Suppose that we have two implementations of RuntimeExecutor: one that is optimized, and sometimes executes code synchronously, and the other, which isn't, and always executes asynchronously. Then, code that works with one implementation might not necessarily work with the other. This is not an issue if we only ever expect to have one implementation of RuntimeExecutor. Then, we could simply depend on the implementation's behaviour. If, however, RuntimeExecutor is supposed to be a generic interface, then I think simply allowing the implementation to decided when it's async vs sync makes the RuntimeExecutor abstraction unreliable. Without knowledge of whether the work will be executed asynchronously/synchronously, we'll have to write code that works in both cases, which is hard if the work performs side-effects. Worse, if the behaviour of RuntimeExecutor is ambiguous, we might end up (accidentally) writing code that depends on a particular implementation's behaviours. This is just bad no matter how you slice it.
🤔... You're right. If we always make it async, then this would be problematic. Always-async runtime executor doesn't work. So it looks like we have two options:
|
My point is that those cases are indistinguishable: if some implementation runs the code sometimes synchronously it can have exact same side-effects as always-async one and vise-versa. If some code relies on assumptions related to that, it's probably already broken. I think the another guarantees that |
@RSNara, for what it's worth, this is already a core aspect of React itself. Calls to |
I want to preface my comment by saying I've only recently done a deeper look into React Native's internals and my C++ is very rusty. But I can talk more from the perspective of an end-user, for a use case that I would want to accomplish from the proposed API. I want to be able to delegate certain native lifecycle methods (in this instance, for Enter JSI: I started to dig more into JSI and was finally able to do what I wanted with the raw I'd love to see improved APIs for working with JSI. I wanted to get my particular experience out there so that this use case can be addressed. And perhaps it is already covered by the ideas put forth here. 😅 |
@dvicory Thanks for your comment! That use case is definitely one we want to support - we need to be able to synchronously call into JS (and get a return value) for proper Android back button handling, too. I think we'll support this with the explicitly synchronous API built on top of RuntimeExecutor, like @shergin described above. @RSNara / @shergin : It seems to me that there are 3 potential problems with the sometimes-synchronous RuntimeExecutor:
With the always-async bridge, you can (probably?) be certain that 'Start' will always be logged before 'Done!', because the JS callback will be enqueued after the current JS execution block. With the sometimes-synchronous RuntimeExecutor, it's possible that 'Done!' will come before 'Start' if So this is potentially a problem, if native module methods are invoked synchronously in the JS thread (although IIUC that's not the default behavior for void functions). Outside of this example, I'm struggling to think of example of when this would cause problems for us - Java and Obj-C are both multi-threaded, so it seems to me that any code that's thread-safe should not be making assumptions about where/when JS is being executed. The only places where people might reasonably be making those assumptions are in JS. If that's true, then the only thing we really need to sort out is the JS-native-JS issue, and there may be other ways to solve that than by changing the definition of RuntimeExecutor? On the other hand, I'm also not sure if there's an important reason why the JS queue shouldn't be part of the spec. Even if we want to synchronously execute JS from the main thread, I can't think of a compelling reason why we we need to execute JS on the main thread. And the possibility of using thread pools, etc. doesn't really make sense here since you need exclusive access to the runtime anyway. It just seems like it would be nice to allow the provider of the RuntimeExecutor to do what they want, and not to be tied to a specific thread. |
If Fabric we (will) do that to deliver some events synchronously (e.g. IMHO, this is all about the idea of building something small and extendable, instead of building something complex and immediately versatile. Any constraint that we impose on RuntimeExecutor should serve some need that impossible achieve otherwise. |
For this example, I was thinking about |
I've finally had a chance to catch up on this discussion, and I have a few thoughts.
My intuition is we should start simple and explicit:
This all is, of course, limiting, but that's the point. As we port code over to the new architecture, we will have more understanding of additional use cases we might add:
We can have clearly-labelled experimental APIs to experiment with things we really don't understand well yet.
I'm probably missing use cases. The overall strategy here is to start with simple, safe, orthogonal APIs to make existing simple patterns work. We don't conflate different APIs. We give ourselves a path to more complex patterns which might require more coupling between layers, and/or more complex reasoning about correctness. For simple apps and modules with simple needs, we can keep the older APIs around, and old code just keeps working. Let's not try to get everything perfect from the beginning. |
Hey, Thank you for forming this great proposal and being so explicit about upcoming changes! I have some very basic questions around the idea presented here - I am trying to wrap my head around the latest architecture changes. I've spoken with @ejanzer on Discord and she recommended I ask these questions here - there might be other developers wondering the same, so let's go. I guess my main question is why do we even need to write a new abstraction such as
Shouldn't it be possible to provide thread-safe access to The proposal says:
and later
I guess my main question is what is wrong/no-go with current architecture that would require a whole new set of classes and abstractions? I would say accessing Also, I would like to understand how does the API look on iOS right now and whether it's as hacky as Android. |
The biggest difference between this approach and reusing JSExecutor is that using the You could ask why we can't just build this on top of our existing abstractions, though - why not have JSExecutor provide a RuntimeExecutor? We still might do that for backward compatibility purposes, but for the bridgeless rewrite we think it makes sense to create some new abstractions for a few reasons:
I'm not as familiar with how this works on iOS - I think we pass around the bridge everywhere instead of the runtime pointer, which may not be as dangerous but does present its own challenges for bridgeless mode. |
@mhorowitz I think you're right - we're conflating (at least) two different things here. The API that we expose to third party developers that want to add support for a new JS VM does not have to be the same API that we expose within RN core and to third party code that needs access to the runtime - to your point, the API that we use for accessing the runtime needs to have some logic for handling exceptions, thread management, etc. That logic should probably be shared between all different JS VMs; we don't want to have to implement it multiple times, or ask third party developers to implement it for the VMs they want to support. In particular, the logic for deferring queued work - to be able to insert or prepend a high priority task - would be a pretty significant change, and probably not something we want to require third party developers to implement for the JS engines they want to use. I think this proposal should probably be split into two different APIs:
The first API should only be used by RN core to create the abstraction for the second API. In the future, we can build additional APIs on top of these - like the ones you listed, as well as:
For this proposal, I'd like to figure out what goes in the specs for the first two abstractions. Should they both have the same RuntimeExecutor function signature, with the understanding that one is sync and unsafe, and the other is async and safe? Or should we call them different things, for clarity? If the first one is always synchronous, should it still guarantee exclusive access via locking? Or should we rely on the second abstraction to provide exclusive access through the JS message queue thread (or whatever mechanism we want in the future)?
*Always async because it enqueues the callable on the JS message queue thread, even when it's called from that thread - I believe this is the current behavior. |
Marc, I do like your idea about the always-sync RuntimeExecutor by default as the simplest possible concept. That would work for me too. The reason why I advocate for unspecified behavior for RuntimeExecutor is to actually allow us to make it sync in the future. The problem is that currently, with existing dispatching infrastructure (several of them, one per platform), it's impossible to implement always-sync RuntimeExecutor efficiently, so the idea is to implement it somehow, and then build on top of that what we need. So, having RuntimeExecutor with unspecified dispatching we can have tools (functions like We need to figure out the propagation of the exception indeed. I don't have concrete ideas about that besides "let's propagate them". (I think the question is: how exactly.) Emily, |
Thank you @ejanzer for answering my questions! The three points that you have mentioned are exactly the context I was looking for. I really appreciate this. |
Any news about this ? Am I wrong in thinking that this is a step stone for proper Turbo Modules on RN ? Even proper concurrent mode ? Pretty clear that external events wreak havoc on plans, I'm just kind of thinking out loud. |
@jbrodriguez No updates at the moment, I'm still tinkering with this to find the right API for what we need. This proposal isn't blocking any work on Fabric or TurboModules, which both have their own way of accessing the runtime through the bridge. We want to make this change for bridgeless RN, but that'll come after Fabric + TM. |
Summary: In https://phab.comm.dev/D4650 we enabled informative JSError to be thrown when SQLite query fails on databaseThread of CommCoreModule. I tested it only on emulators and it worked. However on physical devices I found out that I cannot access jsi::Runtime passed to function from auxiliary thread. Context is here react-native-community/discussions-and-proposals#196. This diff updates the code so that we only catch C++ error in database thread and transform it to JSError on the main thread. This change also solves the crash described here: https://linear.app/comm/issue/ENG-1714/ashoat-experienced-5-crashes-in-one-day-on-build-142 Test Plan: Place temporary 'throw std::system_error(ECANCELED, std::generic_category(), "error");' at the begining of SQLiteWueryExecutor::getAllMessages. Build and start the app. Without this diff it will crash. With this diff it will throw informative JSError. Reviewers: tomek, atul, jon Reviewed By: tomek Subscribers: ashoat, abosh Differential Revision: https://phab.comm.dev/D4976
Summary: In https://phab.comm.dev/D4650 we enabled informative JSError to be thrown when SQLite query fails on databaseThread of CommCoreModule. I tested it only on emulators and it worked. However on physical devices I found out that I cannot access jsi::Runtime passed to function from auxiliary thread. Context is here react-native-community/discussions-and-proposals#196. This diff updates the code so that we only catch C++ error in database thread and transform it to JSError on the main thread. This change also solves the crash described here: https://linear.app/comm/issue/ENG-1714/ashoat-experienced-5-crashes-in-one-day-on-build-142 Test Plan: Place temporary 'throw std::system_error(ECANCELED, std::generic_category(), "error");' at the begining of SQLiteWueryExecutor::getAllMessages. Build and start the app. Without this diff it will crash. With this diff it will throw informative JSError. Reviewers: tomek, atul, jon Reviewed By: tomek Subscribers: ashoat, abosh Differential Revision: https://phab.comm.dev/D4976
Hi guys, just here to ask what's the current state of this. With JSI do we have a way to use runtime in a thread safe manner? |
Hi, did you find a way? I am in need of something similar(receive data from tcp sockets on c++, and need to pass it to js in async way). |
@maksimlya hi, in the end i found this guy on youtube that showed how to do that. Basically you have to implement your own thread pool to queue the async work, make use of the jsCallInvoker and implement manually the promise js object in the cpp side. https://youtu.be/SC9PwcKw20o?si=gkt2K_OqrcMwUuZ4 |
Thx alot!. I've already done everything regarding thread pool and async work, just couldn't get to how to return result to js from different thread. Will check it out. |
Introduction
We're currently rewriting core pieces of React Native's infrastructure to function without the bridge. As part of that effort, we want to re-think how we create and manage access to the JS runtime.
Right now, the runtime is managed by a JSExecutor, which is created by a JSExecutorFactory, which is passed in to the bridge when it is constructed. Each type of JS VM has its own executor factory (JSC, Hermes, etc.). The JSExecutor is owned by the NativeToJSBridge, which ensures that all operations on the JS runtime take place on the JS message queue thread. The NativeToJSBridge is then owned by the Instance, which interfaces with platform-specific code.
The downside with this approach is that accessing the
jsi::Runtime
is difficult. Adding a new method that accesses the JS runtime and exposing it to platform-specific code requires touching a half dozen classes, or more. So what ends up happening instead is that we access the runtime in unsafe ways, for convenience. For example, on Android we expose the raw pointer to thejsi::Runtime
throughCatalystInstance.getJavaScriptContextHolder()
, which then allows us to access thejsi::Runtime
without the safety of the JS message queue thread, which is used to prevent concurrent access. This has already caused problems for us.The Core of It
Our goal with this proposal is to build the simplest possible abstraction to expose safe access to a
jsi::Runtime
. Once we have this abstraction, we can build higher-level, more versatile interfaces on top of it, which will be exposed to product code (and maybe used within React Native core, as well). The abstraction that we’ve decided to use is what we’re calling RuntimeExecutor.RuntimeExecutor is already used by Fabric today. It's defined in RuntimeExecutor.h:
The RuntimeExecutor must make the following guarantees (note: these are slightly different from what's described in the docblock above):
jsi::Runtime
.The RuntimeExecutor will be created by a class that implements the interface JSEngineInstance. (This interface is not on GitHub yet, but it basically just defines a method,
getRuntimeExecutor
, that returns a RuntimeExecutor). Each JS VM will need its own implementation of this interface (HermesInstance, JSCInstance, etc.).The JSEngineInstance will be the one responsible for the synchronization that guarantees exclusive access to the runtime. It will create the RuntimeExecutor pass it to React Native core when the React Native instance is initialized.
In theory, the JSEngineInstance can use whatever mechanism it wants to guarantee exclusive access to the runtime, as long as it satisfies the above conditions. In practice, however, we will continue to use the JS message queue thread for this. This is important because the assumption that we’re using this thread is baked in to React Native in various places (see discussion).
Here is an example implementation of RuntimeExecutor, from Fabric’s Binding.cpp:
Why Make This Change
The main reason to make this change is because it will guarantee safe (exclusive) access to the
jsi::Runtime
, which we don't currently have in places where store ajsi::Runtime *
. This will prevent crashes caused by accessing the runtime simultaneously from multiple threads.With RuntimeExecutor, it will also be easier to lazily initialize the
jsi::Runtime
than it is today. Right now we don’t have a lot of control over when we pay the cost of initializing the JSExecutorFactory and the runtime. With this model, it will be easier to only initialize the runtime when it’s actually needed.Because we are now delegating threading/synchronization to the JSEngineInstance, it will also be possible for different VMs to use different threading models. For example, on iOS 11 and below (IIRC, needs citation), JSC can only be accessed from the same thread it was created on; other JS VMs don’t have this limitation. By delegating the threading model to the JSEngineInstance, we could have greater flexibility in which thread(s) we use for JS execution.
Discussion points
jsi::Runtime
). However RuntimeExecutor does not provide any other high-level concepts such as serial queues or guaranteed sync or async execution of the callback. Those features can be built on top of RuntimeExecutor in a platform/VM-agnostic manner.std::function
is guaranteed by C++ standard library; the rest is justjsi::Runtime
. All actual complexity of implementation design is hidden behind stablestd::function
interface.Appendix: Possible Implementations
The text was updated successfully, but these errors were encountered: