Rename makeCriticalNativeMethod to discourage over-use

Summary: "Critical" or "Fast" JNI methods are enticing by their name, but carry dangers that are not trivially visible.

Reviewed By: davidaurelio

Differential Revision: D14184560

fbshipit-source-id: 89ec70f53bb2cb89ff568d8b1fe222ede86c9824

ReactAndroid/src/main/jni/first-party/fb/include/fb/fbjni/CoreClasses.h

@@ -235,9 +235,9 @@ class FBEXPORT JClass : public JavaClass<JClass, JObject, jclass> {
   ///     makeNativeMethod("nativeMethodWithExplicitDescriptor",
   ///                      "(Lcom/facebook/example/MyClass;)V",
   ///                      methodWithExplicitDescriptor),
-  ///     makeCriticalNativeMethod("criticalNativeMethodWithAutomaticDescriptor",
+  ///     makeCriticalNativeMethod_DO_NOT_USE_OR_YOU_WILL_BE_FIRED("criticalNativeMethodWithAutomaticDescriptor",
   ///                              criticalNativeMethodWithAutomaticDescriptor),
-  ///     makeCriticalNativeMethod("criticalNativeMethodWithExplicitDescriptor",
+  ///     makeCriticalNativeMethod_DO_NOT_USE_OR_YOU_WILL_BE_FIRED("criticalNativeMethodWithExplicitDescriptor",
   ///                              "(IIF)Z",
   ///                              criticalNativeMethodWithExplicitDescriptor),
   ///  });

ReactAndroid/src/main/jni/first-party/fb/include/fb/fbjni/Registration.h

@@ -84,6 +84,55 @@ struct CriticalMethod<R(*)(Args...)> {
 // signature with an exclamation mark.
 // Android v8+ supports fast calls by annotating methods:
 // https://source.android.com/devices/tech/dalvik/improvements#faster-native-methods
+//
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+//
+// "Fast" calls are only on the order of a few dozen NANO-seconds faster than
+// regular JNI calls. If your method does almost aaanything of consequence - if
+// you loop, if you write to a log, if you call another method, if you even
+// simply allocate or deallocate - then the method body will significantly
+// outweigh the method overhead.
+//
+// The difference between a regular JNI method and a "FastJNI" method (as
+// they're called inside the runtime) is that a FastJNI method doesn't mark the
+// thread as executing native code, and by skipping that avoids the locking and
+// thread state check overhead of interacting with the Garbage Collector.
+//
+// To understand why this is dangerous, you need to understand a bit about the
+// GC. In order to perform its work the GC needs to have at least one (usually
+// two in modern implementations) "stop the world" checkpoints where it can
+// guarantee that all managed-code execution is paused. The VM performs these
+// checkpoints at allocations, method boundaries, and each backward branch (ie
+// anytime you loop). When the GC wants to run, it will signal to all managed
+// threads that they should pause at the next checkpoint, and then it will wait
+// for every thread in the system to transition from the "runnable" state into a
+// "waiting" state. Once every thread has stopped, the GC thread can perform the
+// work it needs to and then it will trigger the execution threads to resume.
+//
+// JNI methods fit neatly into the above paradigm: They're still methods, so
+// they perform GC checkpoints at method entry and method exit. JNI methods also
+// perform checkpoints at any JNI boundary crossing - ie, any time you call
+// GetObjectField etc. Because access to managed objects from native code is
+// tightly controlled, the VM is able to mark threads executing native methods
+// into a special "native" state which the GC is able to ignore: It knows they
+// can't touch managed objects (without hitting a checkpoint) so it doesn't care
+// about them.
+//
+// JNI critical methods don't perform that "runnable" -> "native" thread state
+// transition. Skipping that transition allows them to shave about 20ns off
+// their total execution time, but it means that the GC has to wait for them to
+// complete before it can move forward. If a critical method begins blocking,
+// say on a long loop, or an I/O operation, or on perhaps a mutex, then the GC
+// will also block, and because the GC is blocking the entire rest of the VM
+// (which is waiting on the GC) will block. If the critical method is blocking
+// on a mutex that's already held by the GC - for example, the VM's internal
+// weak_globals_lock_ which guards modifications to the weak global reference
+// table (and is required in order to create or free a weak_ref<>) - then you
+// have a system-wide deadlock.
 
 // prefixes a JNI method signature as android "fast call".
 #if defined(__ANDROID__) && defined(FBJNI_WITH_FAST_CALLS)
@@ -105,7 +154,14 @@ struct CriticalMethod<R(*)(Args...)> {
     func)
 
 #define makeCriticalNativeMethodN(a, b, c, count, ...) makeCriticalNativeMethod ## count
-#define makeCriticalNativeMethod(...) makeCriticalNativeMethodN(__VA_ARGS__, 3, 2)(__VA_ARGS__)
+
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// YOU ALMOST CERTAINLY DO NOT NEED THIS AND IT IS DANGEROUS.
+// See above for an explanation.
+#define makeCriticalNativeMethod_DO_NOT_USE_OR_YOU_WILL_BE_FIRED(...) makeCriticalNativeMethodN(__VA_ARGS__, 3, 2)(__VA_ARGS__)
 
 }}
 

ReactAndroid/src/main/jni/first-party/yogajni/jni/YGJNI.cpp

@@ -719,7 +719,7 @@ jint jni_YGNodeGetInstanceCount() {
 }
 
 #define YGMakeNativeMethod(name) makeNativeMethod(#name, name)
-#define YGMakeCriticalNativeMethod(name) makeCriticalNativeMethod(#name, name)
+#define YGMakeCriticalNativeMethod(name) makeCriticalNativeMethod_DO_NOT_USE_OR_YOU_WILL_BE_FIRED(#name, name)
 
 jint JNI_OnLoad(JavaVM* vm, void*) {
   return initialize(vm, [] {