Page MenuHomePhabricator

Implement JNI Helper for GlobalDBSingleton
ClosedPublic

Authored by marcin on Sep 27 2022, 5:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:11 PM
Unknown Object (File)
Sat, Nov 9, 8:09 PM
Unknown Object (File)
Sat, Nov 9, 6:13 PM
Subscribers

Details

Summary

This differential exposes GlobalDbSingleton API to Java native code through JNI. I understand that this differential is quite big, but all that actually matters is the .cpp file. The rest is just an API definition that is implemented in .cpp file.

Test Plan

At this moment any Java lambda that takes no arguments and returns nothing (void()) can be scheduled on the database thread and executed on the C++ side.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I'll defer to others for a blocking review, but doesn't look wrong to me.

tomek added inline comments.
native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
8 ↗(On Diff #17107)

Why do we need this?

10–15 ↗(On Diff #17107)

Are all these moves necessary or beneficial?

This revision is now accepted and ready to land.Sep 28 2022, 4:06 AM
native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
8 ↗(On Diff #17107)

Without this line task is a local reference that will be cleaned when this method completes on Java side. If we schedule a lambda to be executed at different thread and return to Java this lambda will be cleaned by the time database thread starts executing it. By making it global it will remain alive until C++ destructs it.

10–15 ↗(On Diff #17107)

Without those moves I was constantly getting an error "Unable to retrieve JNI environment. Is the thread attached?". I think without those moves copy constructor gets called and it requires native API that is accessible only when calling lambda in facebook::jni::ThreadScope::WithClassLoader(...).

We should be really sure about this code. If there are any issues, it will probably cause crashes we don't know about. I encourage everybody on this diff to be extremely thorough. We should be 100% sure of every line here. We should not be guessing, or trying things that seem to work without understanding 100% of why/how they work.

tomek requested changes to this revision.Oct 3 2022, 5:48 AM
tomek added inline comments.
native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
10–15 ↗(On Diff #17107)

I think without those moves copy constructor gets called and it requires native API that is accessible only when calling lambda in facebook::jni::ThreadScope::WithClassLoader(...).

Could you confirm that? This sounds a little suspicious to me - why using copy constructor requires native API? Is it the case when copying other objects? Or is Runnable different from other objects?

This revision now requires changes to proceed.Oct 3 2022, 5:48 AM
marcin requested review of this revision.Oct 3 2022, 8:24 AM
marcin added inline comments.
native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
10–15 ↗(On Diff #17107)

Could you confirm that?

global_ref<T> is defined in JNI as basic_strong_ref<T, GlobalReferenceAllocator> (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/References-forward.h#L75).
GlobalReferenceAllocator uses Environment::current() when creating new reference (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/ReferenceAllocators-inl.h#L81). This throws mentioned error when thread is not attached (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.h#L30)

This sounds a little suspicious to me - why using copy constructor requires native API?

In my honest opinion I do not see reasons to consider it suspicious. Runnable is a JVM reference passed to C++, so it seems understandable that its copy constructor requires native API. I also confirmed this with SWM engineer working for Expo team.

Is it the case when copying other objects? Or is Runnable different from other objects?

What other object do you refer to? Runnable is a reference to object instantiated in Java, so it differs from other objects used in this piece of code that were instantiated in C++ from the beginning.

native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
10–15 ↗(On Diff #17107)

Could you confirm that?

global_ref<T> is defined in JNI as basic_strong_ref<T, GlobalReferenceAllocator> (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/References-forward.h#L75).
basic_strong_ref<T, Alloc> (Alloc here refers to type) has implemented constructor and copy constructor - both of them involve calling constructor/copy constructor of basic_owned_ref<T, Alloc> (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/References.h#L461-L471).
Constructors of basic_owned_ref<T, Alloc> call Alloc{}.newReference() (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/References-inl.h#L211-L242).
newReference() for GlobalReferenceAllocator calls Environment::current() (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/ReferenceAllocators-inl.h#L81).
This throws mentioned exception when calling thread is not attached (https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.h#L30).

tomek added inline comments.
native/android/app/src/cpp/GlobalDBSingletonJNIHelper.cpp
10–15 ↗(On Diff #17107)

Ok, that makes sense. Thanks for this explanation!

This revision is now accepted and ready to land.Oct 4 2022, 4:50 AM
This revision was automatically updated to reflect the committed changes.