Details

Reviewers

atul
• jon
tomek

Commits

rCOMMe5855464dab2: Implement JNI Helper for GlobalDBSingleton

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

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Sep 27 2022, 5:38 AM

Herald added subscribers: • abosh, ashoat. · View Herald TranscriptSep 27 2022, 5:38 AM

marcin added a parent revision: D5235: Implement C++ class to pass Java lambda (Runnable interface implementation) to C++ code).Sep 27 2022, 5:44 AM

Harbormaster completed remote builds in B12458: Diff 17105.Sep 27 2022, 5:49 AM

marcin requested review of this revision.Sep 27 2022, 5:49 AM

marcin edited the summary of this revision. (Show Details)Sep 27 2022, 5:49 AM

marcin added a child revision: D5237: Use GlobalDbSingleton API on Android to update database in reponse to notifs in CommNotificationsHandler..

Add missing newlines

Harbormaster completed remote builds in B12460: Diff 17107.Sep 27 2022, 6:07 AM

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

tomek accepted this revision.Sep 28 2022, 4:06 AM

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

marcin added inline comments.Sep 28 2022, 4:21 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.

marcin added inline comments.Oct 4 2022, 12:40 AM

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).