Cleanup JBackgroundExecutor and support thread naming (#33780)

Summary: Pull Request resolved: #33780 Currently this just has a default ThreadPool name, which can be confusing in systraces and crash reports. Changelog: [Internal] Reviewed By: ryancat Differential Revision: D36200090 fbshipit-source-id: 22918993e7c822ed721ccaf79cdcd9d2a972193d
facebook · May 9, 2022 · 0d9e054 · 0d9e054
1 parent 94341f0
commit 0d9e054
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 25 deletions.
diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/BackgroundExecutor.java
@@ -10,15 +10,32 @@
 import com.facebook.proguard.annotations.DoNotStrip;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
 
 @DoNotStrip
 public class BackgroundExecutor {
   private static final String TAG = "FabricBackgroundExecutor";
+
+  private static class NamedThreadFactory implements ThreadFactory {
+    private final String mName;
+
+    public NamedThreadFactory(String name) {
+      mName = name;
+    }
+
+    @Override
+    public Thread newThread(Runnable r) {
+      Thread thread = Executors.defaultThreadFactory().newThread(r);
+      thread.setName(mName);
+      return thread;
+    }
+  }
+
   private final ExecutorService mExecutorService;
 
   @DoNotStrip
-  private BackgroundExecutor() {
-    mExecutorService = Executors.newFixedThreadPool(1);
+  private BackgroundExecutor(String name) {
+    mExecutorService = Executors.newFixedThreadPool(1, new NamedThreadFactory(name));
   }
 
   @DoNotStrip

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.cpp
@@ -6,8 +6,10 @@
  */
 
 #include "Binding.h"
+
 #include "AsyncEventBeat.h"
 #include "EventEmitterWrapper.h"
+#include "JBackgroundExecutor.h"
 #include "ReactNativeConfigHolder.h"
 #include "StateWrapperImpl.h"
 
@@ -455,8 +457,8 @@ void Binding::installFabricUIManager(
   toolbox.synchronousEventBeatFactory = synchronousBeatFactory;
   toolbox.asynchronousEventBeatFactory = asynchronousBeatFactory;
 
-  backgroundExecutor_ = std::make_unique<JBackgroundExecutor>();
-  toolbox.backgroundExecutor = backgroundExecutor_->get();
+  backgroundExecutor_ = JBackgroundExecutor::create("fabric_bg");
+  toolbox.backgroundExecutor = backgroundExecutor_;
 
   animationDriver_ = std::make_shared<LayoutAnimationDriver>(
       runtimeExecutor, contextContainer, this);
@@ -558,8 +560,7 @@ void Binding::preallocateView(
   };
 
   if (dispatchPreallocationInBackground_) {
-    auto backgroundExecutor = backgroundExecutor_->get();
-    backgroundExecutor(preallocationFunction);
+    backgroundExecutor_(preallocationFunction);
   } else {
     preallocationFunction();
   }

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/Binding.h
@@ -9,6 +9,9 @@
 
 #include "FabricMountingManager.h"
 
+#include <memory>
+#include <mutex>
+
 #include <fbjni/fbjni.h>
 #include <react/jni/JRuntimeExecutor.h>
 #include <react/jni/JRuntimeScheduler.h>
@@ -18,12 +21,9 @@
 #include <react/renderer/scheduler/SchedulerDelegate.h>
 #include <react/renderer/uimanager/LayoutAnimationStatusDelegate.h>
 
-#include <memory>
-#include <mutex>
 #include "ComponentFactory.h"
 #include "EventBeatManager.h"
 #include "EventEmitterWrapper.h"
-#include "JBackgroundExecutor.h"
 #include "SurfaceHandlerBinding.h"
 
 namespace facebook {
@@ -144,7 +144,7 @@ class Binding : public jni::HybridClass<Binding>,
 
   std::shared_ptr<LayoutAnimationDriver> animationDriver_;
 
-  std::unique_ptr<JBackgroundExecutor> backgroundExecutor_;
+  BackgroundExecutor backgroundExecutor_;
 
   butter::map<SurfaceId, SurfaceHandler> surfaceHandlerRegistry_{};
   butter::shared_mutex

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.cpp
@@ -18,16 +18,15 @@ using namespace facebook::jni;
 using facebook::react::JNativeRunnable;
 using facebook::react::Runnable;
 
-BackgroundExecutor JBackgroundExecutor::get() {
-  auto self = JBackgroundExecutor::create();
-
-  return [self](std::function<void()> &&runnable) {
+BackgroundExecutor JBackgroundExecutor::create(const std::string &name) {
+  auto instance = make_global(newInstance(name));
+  return [instance = std::move(instance)](std::function<void()> &&runnable) {
     static auto method =
-        findClassStatic(JBackgroundExecutor::JBackgroundExecutorJavaDescriptor)
-            ->getMethod<void(Runnable::javaobject)>("queueRunnable");
+        javaClassStatic()->getMethod<void(Runnable::javaobject)>(
+            "queueRunnable");
 
     auto jrunnable = JNativeRunnable::newObjectCxxArgs(std::move(runnable));
-    method(self, static_ref_cast<Runnable::javaobject>(jrunnable).get());
+    method(instance, jrunnable.get());
   };
 }
 

diff --git a/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h b/ReactAndroid/src/main/java/com/facebook/react/fabric/jni/JBackgroundExecutor.h
@@ -20,14 +20,7 @@ class JBackgroundExecutor : public JavaClass<JBackgroundExecutor> {
   static auto constexpr kJavaDescriptor =
       "Lcom/facebook/react/bridge/BackgroundExecutor;";
 
-  constexpr static auto JBackgroundExecutorJavaDescriptor =
-      "com/facebook/react/bridge/BackgroundExecutor";
-
-  static global_ref<javaobject> create() {
-    return make_global(newInstance());
-  }
-
-  BackgroundExecutor get();
+  static BackgroundExecutor create(const std::string &name);
 };
 
 } // namespace react