Move sapling_backingstore_get_tree_batch to cxx_bridge

Summary: X-link: facebookincubator/velox#7781 cxx.rs provides a more ergonomic and opinionated interop layer between Rust and C++ that we would like to leverage for future API chages. This moves sapling_backingstore_get_tree_batch from the existing cbindgen implemenation to the new cxxbridge. Reviewed By: MichaelCuevas Differential Revision: D51645547 fbshipit-source-id: 0b11ea745efe02f282200fe5a325422978a1b466
facebook · Nov 29, 2023 · 10d37f5 · 10d37f5
1 parent 3911108
commit 10d37f5
Show file tree

Hide file tree

Showing 11 changed files with 137 additions and 96 deletions.
diff --git a/build/fbcode_builder/CMake/RustStaticLibrary.cmake b/build/fbcode_builder/CMake/RustStaticLibrary.cmake
@@ -363,7 +363,7 @@ endfunction()
 #   `${TARGET}` CMake library target.
 #
 # ```cmake
-# rust_cxx_bridge(<TARGET> <CXX_BRIDGE_FILE> [CRATE <CRATE_NAME>])
+# rust_cxx_bridge(<TARGET> <CXX_BRIDGE_FILE> [CRATE <CRATE_NAME>] [LIBS <LIBNAMES>])
 # ```
 #
 # Parameters:
@@ -374,9 +374,11 @@ endfunction()
 # - CRATE_NAME:
 #   Name of the crate. This parameter is optional. If unspecified, it will
 #   fallback to `${TARGET}`.
+# - LIBS <lib1> [<lib2> ...]:
+#   A list of libraries that this library depends on.
 #
 function(rust_cxx_bridge TARGET CXX_BRIDGE_FILE)
-  fb_cmake_parse_args(ARG "" "CRATE" "" "${ARGN}")
+  fb_cmake_parse_args(ARG "" "CRATE" "LIBS" "${ARGN}")
 
   if(DEFINED ARG_CRATE)
     set(crate_name "${ARG_CRATE}")
@@ -476,6 +478,11 @@ function(rust_cxx_bridge TARGET CXX_BRIDGE_FILE)
     $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
     $<INSTALL_INTERFACE:include>
   )
+  target_link_libraries(
+    ${crate_name}
+    PUBLIC
+    ${ARG_LIBS}
+  )
 
   file(MAKE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/rust")
   add_custom_command(
@@ -517,10 +524,11 @@ function(rust_cxx_bridge TARGET CXX_BRIDGE_FILE)
     COMMENT "Generating cxx bindings for crate ${crate_name}"
   )
 
-  target_sources(${crate_name}
+  target_sources(
+    ${crate_name}
     PRIVATE
-      "${CMAKE_CURRENT_BINARY_DIR}/${cxx_header}"
-      "${CMAKE_CURRENT_BINARY_DIR}/rust/cxx.h"
-      "${CMAKE_CURRENT_BINARY_DIR}/${cxx_source}"
+    "${CMAKE_CURRENT_BINARY_DIR}/${cxx_header}"
+    "${CMAKE_CURRENT_BINARY_DIR}/rust/cxx.h"
+    "${CMAKE_CURRENT_BINARY_DIR}/${cxx_source}"
   )
 endfunction()
diff --git a/eden/fs/store/hg/HgDatapackStore.cpp b/eden/fs/store/hg/HgDatapackStore.cpp
@@ -136,6 +136,23 @@ void HgDatapackStore::getTreeBatch(const ImportRequestsList& importRequests) {
       [&, filteredPaths](
           size_t index,
           folly::Try<std::shared_ptr<sapling::Tree>> content) mutable {
+        if (content.hasException()) {
+          XLOGF(
+              DBG6,
+              "Failed to import node={} from EdenAPI (batch tree {}/{}): {}",
+              folly::hexlify(requests[index]),
+              index,
+              requests.size(),
+              content.exception().what().toStdString());
+        } else {
+          XLOGF(
+              DBG6,
+              "Imported node={} from EdenAPI (batch tree: {}/{})",
+              folly::hexlify(requests[index]),
+              index,
+              requests.size());
+        }
+
         if (config_->getEdenConfig()->hgTreeFetchFallback.getValue() &&
             content.hasException()) {
           if (logger_) {

diff --git a/eden/scm/lib/backingstore/CMakeLists.txt b/eden/scm/lib/backingstore/CMakeLists.txt
@@ -19,19 +19,20 @@ install_rust_static_library(
   INSTALL_DIR lib
 )
 
-rust_cxx_bridge(rust_backingstore_bridge "src/ffi.rs")
+rust_cxx_bridge(
+  backingstore
+  "src/ffi.rs"
+  LIBS
+  fmt::fmt
+  Folly::folly
+)
 
 file(GLOB C_API_SRCS "src/*.cpp")
-file(
-  GLOB
-  RUST_BACKINGSTORE_SRCS
-  "${CMAKE_CURRENT_BINARY_DIR}/cxxbridge/backingstore/src/*.cc"
-)
 file(GLOB C_API_HDRS "include/*.h")
-add_library(
+target_sources(
   backingstore
+  PRIVATE
   "${C_API_SRCS}"
-  "${RUST_BACKINGSTORE_SRCS}"
 )
 set_target_properties(
   backingstore
@@ -40,17 +41,18 @@ set_target_properties(
   "${C_API_HDRS}"
 )
 
-target_include_directories(backingstore PUBLIC
+target_include_directories(
+  backingstore
+  PUBLIC
   $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
   $<INSTALL_INTERFACE:include>
 )
 target_link_libraries(
   backingstore
   PUBLIC
   rust_backingstore
-  rust_backingstore_bridge
+  fmt::fmt
   Folly::folly
-  edencommon::edencommon_utils
 )
 
 # curl used in the Rust crate has its own copy of curl compiled and it uses

diff --git a/eden/scm/lib/backingstore/include/BackingStoreBindings.h b/eden/scm/lib/backingstore/include/BackingStoreBindings.h
@@ -7,7 +7,7 @@
  * This file is generated with cbindgen. Please run `./tools/cbindgen.sh` to
  * update this file.
  *
- * @generated SignedSource<<acd2733514d3f5e17ff03431ac01b0a3>>
+ * @generated SignedSource<<3b2996b92f3ae83007b1c8ab31ad87b4>>
  *
  */
 
@@ -28,7 +28,7 @@ struct BackingStore;
 
 struct FileAuxData;
 
-struct Tree;
+struct Request;
 
 template<typename T = void>
 struct Vec;
@@ -46,10 +46,6 @@ struct CBytes {
   }
 };
 
-struct Request {
-  const uint8_t *node;
-};
-
 template<typename T>
 struct Slice {
   const T *ptr;
@@ -77,12 +73,6 @@ void sapling_cbytes_free(CBytes *vec);
 
 void sapling_cfallible_free_error(char *ptr);
 
-void sapling_backingstore_get_tree_batch(BackingStore *store,
-                                         Slice<Request> requests,
-                                         bool local,
-                                         void *data,
-                                         void (*resolve)(void*, uintptr_t, CFallibleBase));
-
 void sapling_backingstore_get_blob_batch(BackingStore *store,
                                          Slice<Request> requests,
                                          bool local,
@@ -107,8 +97,6 @@ CFallibleBase sapling_test_cfallible_err();
 
 CBytes sapling_test_cbytes();
 
-void sapling_tree_free(Tree *tree);
-
 } // extern "C"
 
 } // namespace sapling

diff --git a/eden/scm/lib/backingstore/include/SaplingNativeBackingStore.h b/eden/scm/lib/backingstore/include/SaplingNativeBackingStore.h
@@ -23,11 +23,6 @@ class IOBuf;
 
 namespace sapling {
 
-class SaplingFetchError : public std::runtime_error {
- public:
-  using std::runtime_error::runtime_error;
-};
-
 /**
  * Reference to a 20-byte hg node ID.
  *

diff --git a/eden/scm/lib/backingstore/include/ffi.h b/eden/scm/lib/backingstore/include/ffi.h
@@ -11,4 +11,31 @@
 #include <rust/cxx.h>
 #include <memory>
 
-namespace sapling {} // namespace sapling
+namespace sapling {
+
+class SaplingFetchError : public std::runtime_error {
+ public:
+  using std::runtime_error::runtime_error;
+};
+
+struct Tree;
+
+/**
+ * Resolver used in the processing of getTreeBatch requests.
+ */
+struct GetTreeBatchResolver {
+  explicit GetTreeBatchResolver(
+      folly::FunctionRef<void(size_t, folly::Try<std::shared_ptr<Tree>>)>
+          resolve)
+      : resolve{std::move(resolve)} {}
+
+  folly::FunctionRef<void(size_t, folly::Try<std::shared_ptr<Tree>>)> resolve;
+};
+
+void sapling_backingstore_get_tree_batch_handler(
+    std::shared_ptr<GetTreeBatchResolver> resolver,
+    size_t index,
+    rust::String error,
+    std::shared_ptr<Tree> tree);
+
+} // namespace sapling
diff --git a/eden/scm/lib/backingstore/src/SaplingNativeBackingStore.cpp b/eden/scm/lib/backingstore/src/SaplingNativeBackingStore.cpp
@@ -6,6 +6,7 @@
  */
 
 #include "eden/scm/lib/backingstore/include/SaplingNativeBackingStore.h"
+#include "eden/scm/lib/backingstore/src/ffi.rs.h" // @manual
 
 #include <folly/Range.h>
 #include <folly/String.h>
@@ -93,7 +94,8 @@ void SaplingNativeBackingStore::getTreeBatch(
     bool local,
     folly::FunctionRef<void(size_t, folly::Try<std::shared_ptr<Tree>>)>
         resolve) {
-  size_t count = requests.size();
+  auto resolver = std::make_shared<GetTreeBatchResolver>(std::move(resolve));
+  auto count = requests.size();
 
   XLOG(DBG7) << "Import batch of trees with size:" << count;
 
@@ -105,41 +107,11 @@ void SaplingNativeBackingStore::getTreeBatch(
     });
   }
 
-  using ResolveResult = folly::Try<std::shared_ptr<Tree>>;
-
-  auto inner_resolve = [&](size_t index, CFallibleBase raw_result) {
-    resolve(
-        index, folly::makeTryWith([&] {
-          CFallible<Tree, sapling_tree_free> result{std::move(raw_result)};
-          if (result.isError()) {
-            XLOGF(
-                DBG6,
-                "Failed to import node={} from EdenAPI (batch tree {}/{}): {}",
-                folly::hexlify(requests[index]),
-                index,
-                count,
-                result.getError());
-            return ResolveResult{SaplingFetchError{result.getError()}};
-          } else {
-            XLOGF(
-                DBG6,
-                "Imported node={} from EdenAPI (batch tree: {}/{})",
-                folly::hexlify(requests[index]),
-                index,
-                count);
-            return ResolveResult{std::shared_ptr<Tree>{result.unwrap()}};
-          }
-        }));
-  };
-
   sapling_backingstore_get_tree_batch(
-      store_.get(),
-      folly::crange(raw_requests),
+      *store_.get(),
+      rust::Slice<const Request>{raw_requests.data(), raw_requests.size()},
       local,
-      &inner_resolve,
-      +[](void* fn, size_t index, CFallibleBase result) {
-        (*static_cast<decltype(inner_resolve)*>(fn))(index, result);
-      });
+      std::move(resolver));
 }
 
 std::unique_ptr<folly::IOBuf> SaplingNativeBackingStore::getBlob(

diff --git a/eden/scm/lib/backingstore/src/ffi.cpp b/eden/scm/lib/backingstore/src/ffi.cpp
@@ -7,4 +7,22 @@
 
 #include "eden/scm/lib/backingstore/include/ffi.h"
 
-namespace sapling {} // namespace sapling
+namespace sapling {
+
+void sapling_backingstore_get_tree_batch_handler(
+    std::shared_ptr<GetTreeBatchResolver> resolver,
+    size_t index,
+    rust::String error,
+    std::shared_ptr<Tree> tree) {
+  using ResolveResult = folly::Try<std::shared_ptr<Tree>>;
+
+  resolver->resolve(index, folly::makeTryWith([&] {
+                      if (tree) {
+                        return ResolveResult{tree};
+                      } else {
+                        return ResolveResult{SaplingFetchError{error.c_str()}};
+                      }
+                    }));
+}
+
+} // namespace sapling