add a executor for checkoutRevision endpoint

Summary:
# Context
We’ve encountered issues in the past of certain functions having wide execution fan-out, meaning one Thrift call could tie up all of the Thrift CPU threads to serve the request. This is not ideal, as this results in other clients receiving `QueueTimeout`s due to waiting too long to get Thrift requests picked up for execution. We’ve combated this in the past by limiting some Thrift entry points to serial execution. This has become a game of whack-a-mole with new instances of `QueueTimeout`s, requiring copying this logic to different functions that seem to have similar fan-out, and is error prone since its easy to forget to set up a SerialExecutor while refactoring pre-existing functions.

There are some mission critical entry points that are known to be more heavyweight and should not be executed serially. There are a few options to be explored for these cases, including adding thread pools for particular heavyweight entry points, or implementing a Executor which can use smarter internal load balancing, such as a token bucket backed solution. This diff explores the first option.

# This Diff
This diff adds a `UnboundedQueueExecutor` that can be used to drive `checkout()`. Without this, and while using serial execution for Thrift requests across the board, checkout becomes too slow. This allows us to use serial execution for everything while keeping `checkoutRevision` performant. We might need to introduce similar executors for other endpoints, TBD.

# Technical Details
The use of a `UnboundedQueueExecutor` is explicit here. I plan to go through and remove uses of bounded executors in EdenFS. It is not recommended to use bounded executors with async code [1]

# Facebook
[1] - https://fb.workplace.com/groups/474291069286180/posts/5360947373953834/?comment_id=5361092820605956

[Threadpool Project Plan](https://docs.google.com/document/d/1yvYFYk8UT3hW9M7Qw-NSIpvqYFUxjHL8FhM81cYQGeQ/edit?tab=t.0)

Reviewed By: jdelliot

Differential Revision: D70135765

fbshipit-source-id: 66634542dbc3c69e8bb2c75ab071313e0ab37f80

Author: genevievehelsel

eden/fs/config/EdenConfig.h

@@ -328,6 +328,27 @@ class EdenConfig : private ConfigSettingManager {
       false,
       this};
 
+  /**
+   * Whether Eden should use a dedicated executor for checkout requests. This is
+   * meant to help with checkoutRevision performance while using serial
+   * execution for other Thrift requests. This feature can be used even if not
+   * using serial execution for other Thrift requests, but if serial execution
+   * is being used, its a good idea to turn on this config as well.
+   */
+  ConfigSetting<bool> thriftUseCheckoutExecutor{
+      "thrift:use-checkout-executor",
+      false,
+      this};
+
+  /**
+   * Number of threads that will service the checkoutRevision Thrift endpoint
+   * when using its own executor.
+   */
+  ConfigSetting<uint64_t> numCheckoutThreads{
+      "thrift:checkout-revision-num-servicing-threads",
+      std::thread::hardware_concurrency(),
+      this};
+
   /**
    * How often to collect Thrift server metrics. The default value mirrors the
    * value from facebook::fb303::TServerCounters::kDefaultSampleRate

eden/fs/service/EdenServer.cpp

@@ -343,6 +343,17 @@ std::shared_ptr<folly::Executor> makeFsChannelThreads(
   return fsChannelThreads;
 }
 
+std::shared_ptr<folly::Executor> makeCheckoutRevisionThreads(
+    bool useCheckoutExecutor,
+    std::shared_ptr<const EdenConfig>& edenConfig) {
+  if (useCheckoutExecutor) {
+    return std::make_shared<UnboundedQueueExecutor>(
+        edenConfig->numCheckoutThreads.getValue(),
+        "CheckoutRevisionThreadPool");
+  }
+  return nullptr;
+}
+
 } // namespace
 
 namespace facebook::eden {
@@ -486,6 +497,10 @@ EdenServer::EdenServer(
       thriftUseResourcePools_{edenConfig->thriftUseResourcePools.getValue()},
       thriftUseSerialExecution_{
           edenConfig->thriftUseSerialExecution.getValue()},
+      thriftUseCheckoutExecutor_{
+          edenConfig->thriftUseCheckoutExecutor.getValue()},
+      checkoutRevisionExecutor_{
+          makeCheckoutRevisionThreads(thriftUseCheckoutExecutor_, edenConfig)},
       progressManager_{
           std::make_unique<folly::Synchronized<EdenServer::ProgressManager>>()},
       startupStatusChannel_{std::move(startupStatusChannel)} {
@@ -2254,6 +2269,26 @@ ImmediateFuture<CheckoutResult> EdenServer::checkOutRevision(
             return std::move(result);
           });
 
+  if (thriftUseCheckoutExecutor_) {
+    // This is an UnboundedQueueExecutor, which is guaranteed to never block
+    // nor throw (except OOM), nor execute inline from `add()`.
+    //
+    // Thrift documentation states that "it is almost never a good idea to send
+    // work off Thrift’s CPU worker". However, it also notes that "user code may
+    // delegate the rest of the work to other thread pools, thus shifting load
+    // off Thrift’s CPU Workers thread and letting the thread pick up new
+    // incoming work" but warns that "if there is work offloaded to other thread
+    // pools, there should be backpressure mechanism that would block Thrift CPU
+    // Workers thread when those internal thread pools are overloaded."
+    //
+    // Without backpressure, EdenFS would see an increase in memory pressure.
+    // potential slowdowns, and/or OOMS. EdenFS only allows one checkout at a
+    // time (https://fburl.com/code/a6eoy8wu), which acts as the aformentioned
+    // backpressure mechanism.
+    checkoutFuture =
+        std::move(checkoutFuture).semi().via(checkoutRevisionExecutor_.get());
+  }
+
   return std::move(checkoutFuture).ensure([mountHandle] {});
 }
 

eden/fs/service/EdenServer.h

@@ -799,6 +799,19 @@ class EdenServer : private TakeoverHandler {
    */
   bool thriftUseSerialExecution_;
 
+  /**
+   * If the Thrift Server created at startup is configured to use a dedicated
+   * executor for the checkoutRevision endpoint.
+   */
+  bool thriftUseCheckoutExecutor_;
+
+  /**
+   * Dedicated executor for checkoutRevision Thrift endpoint. This will be null
+   * if the config to use a dedicated executor is not set at daemon startup, if
+   * thriftUseCheckoutExecutor_ is false.
+   */
+  std::shared_ptr<folly::Executor> checkoutRevisionExecutor_;
+
   /**
    * Remounting progress state.
    */

eden/integration/hg/update_test.py

@@ -1473,3 +1473,62 @@ def stop_eden() -> None:
             # throws if eden exits uncleanly - which it does if there is some
             # sort of crash.
             shutdown_thread.join()
+
+
+@hg_test
+# pyre-ignore[13]: T62487924
+class UpdateDedicatedExecutorTest(EdenHgTestCase):
+    # pyre-fixme[13]: Attribute `commit1` is never initialized.
+    commit1: str
+    # pyre-fixme[13]: Attribute `commit2` is never initialized.
+    commit2: str
+    # pyre-fixme[13]: Attribute `commit3` is never initialized.
+    commit3: str
+    enable_fault_injection: bool = True
+
+    def edenfs_logging_settings(self) -> Dict[str, str]:
+        return {
+            "eden.fs.inodes.TreeInode": "DBG5",
+            "eden.fs.inodes.CheckoutAction": "DBG5",
+            "eden.fs.inodes.CheckoutContext": "DBG5",
+        }
+
+    def edenfs_extra_config(self) -> Optional[Dict[str, List[str]]]:
+        parent_config = super().edenfs_extra_config()
+        if parent_config is None:
+            parent_config = {}
+        if "thrift" not in parent_config:
+            parent_config["thrift"] = []
+
+        parent_config["thrift"].append("use-checkout-executor = true")
+
+        return parent_config
+
+    def populate_backing_repo(self, repo: hgrepo.HgRepository) -> None:
+        repo.write_file("hello.txt", "hola")
+        repo.write_file(".gitignore", "ignoreme\n")
+        repo.write_file("foo/.gitignore", "*.log\n")
+        repo.write_file("foo/bar.txt", "test\n")
+        repo.write_file("foo/subdir/test.txt", "test\n")
+        self.commit1 = repo.commit("Initial commit.")
+
+        repo.write_file("foo/.gitignore", "*.log\n/_*\n")
+        self.commit2 = repo.commit("Update foo/.gitignore")
+
+        repo.write_file("foo/bar.txt", "updated in commit 3\n")
+        self.commit3 = repo.commit("Update foo/.gitignore")
+
+    def test_checkout_on_dedicated_executor(self) -> None:
+        """Test that checkout can be completed on a dedicated executor."""
+        self.assert_status_empty()
+
+        self.chmod("hello.txt", 0o755)
+        self.assert_status({"hello.txt": "M"})
+        commit4 = self.repo.commit("Update hello.txt mode")
+
+        self.repo.update(self.commit1)
+        self.repo.update(commit4)
+        self.touch("hello.txt")
+        self.repo.update(self.commit1)
+        self.repo.update(commit4)
+        self.assert_status_empty()