Optimize TLS lookup in HgImporter

Summary:
D14677339 added tracing to all HgImporter requests. Each trace loads a thread-local variable through EdenStats::getStatsForCurrentThread(). Each of these loads has a small but non-trivial cost.

Because each instance of HgImporter is used only on one thread, each getStatsForCurrentThread() call returns the same EdenThreadStats object. Avoid the thread-local-variable lookups by caching the result of getStatsForCurrentThread() in HgImporter.

This diff should not change behavior.

Reviewed By: simpkins

Differential Revision: D14794284

fbshipit-source-id: d1609a1720d44c680dc0ebaa2536779def2a6f37

eden/fs/store/hg/HgBackingStore.cpp

@@ -131,8 +131,8 @@ class HgImporterThreadFactory : public folly::ThreadFactory {
 
   std::thread newThread(folly::Func&& func) override {
     return delegate_.newThread([this, func = std::move(func)]() mutable {
-      threadLocalImporter.reset(
-          new HgImporterManager(repository_, localStore_, stats_));
+      threadLocalImporter.reset(new HgImporterManager(
+          repository_, localStore_, getSharedStatsForCurrentThread(stats_)));
       func();
     });
   }
@@ -270,7 +270,8 @@ HgBackingStore::HgBackingStore(
 #ifndef EDEN_WIN_NO_RUST_DATAPACK
   initializeDatapackImport(repository);
 #endif
-  HgImporter importer(repository, localStore, std::move(stats));
+  HgImporter importer(
+      repository, localStore, getSharedStatsForCurrentThread(stats));
   const auto& options = importer.getOptions();
   initializeTreeManifestImport(options, repository);
   repoName_ = options.repoName;

eden/fs/store/hg/HgImporter.cpp

@@ -256,7 +256,7 @@ class HgImporterEofError : public HgImporterError {
 HgImporter::HgImporter(
     AbsolutePathPiece repoPath,
     LocalStore* store,
-    std::shared_ptr<EdenStats> stats,
+    std::shared_ptr<EdenThreadStats> stats,
     std::optional<AbsolutePath> importHelperScript)
     : repoPath_{repoPath}, store_{store}, stats_{std::move(stats)} {
   std::vector<string> cmd;
@@ -561,8 +561,7 @@ unique_ptr<Blob> HgImporter::importFileContents(Hash blobHash) {
   XLOG(DBG4) << "imported blob " << blobHash << " (" << hgInfo.path() << ", "
              << hgInfo.revHash() << "); length=" << bodyLength;
 
-  stats_->getStatsForCurrentThread().hgBackingStoreGetBlob.addValue(
-      watch.elapsed().count());
+  stats_->hgBackingStoreGetBlob.addValue(watch.elapsed().count());
   return make_unique<Blob>(blobHash, std::move(buf));
 }
 
@@ -713,7 +712,7 @@ HgImporter::ChunkHeader HgImporter::readChunkHeader(
 HgImporter::TransactionID HgImporter::sendManifestRequest(
     folly::StringPiece revName) {
 #if defined(EDEN_HAVE_STATS)
-  stats_->getStatsForCurrentThread().hgImporterManifest.addValue(1);
+  stats_->hgImporterManifest.addValue(1);
 #endif
 
   auto txnID = nextRequestID_++;
@@ -736,8 +735,7 @@ HgImporter::TransactionID HgImporter::sendManifestRequest(
 HgImporter::TransactionID HgImporter::sendManifestNodeRequest(
     folly::StringPiece revName) {
 #if defined(EDEN_HAVE_STATS)
-  stats_->getStatsForCurrentThread().hgImporterManifestNodeForCommit.addValue(
-      1);
+  stats_->hgImporterManifestNodeForCommit.addValue(1);
 #endif
 
   auto txnID = nextRequestID_++;
@@ -761,7 +759,7 @@ HgImporter::TransactionID HgImporter::sendFileRequest(
     RelativePathPiece path,
     Hash revHash) {
 #if defined(EDEN_HAVE_STATS)
-  stats_->getStatsForCurrentThread().hgImporterCatFile.addValue(1);
+  stats_->hgImporterCatFile.addValue(1);
 #endif
 
   auto txnID = nextRequestID_++;
@@ -787,7 +785,7 @@ HgImporter::TransactionID HgImporter::sendFileRequest(
 HgImporter::TransactionID HgImporter::sendPrefetchFilesRequest(
     const std::vector<std::pair<RelativePath, Hash>>& files) {
 #if defined(EDEN_HAVE_STATS)
-  stats_->getStatsForCurrentThread().hgImporterPrefetchFiles.addValue(1);
+  stats_->hgImporterPrefetchFiles.addValue(1);
 #endif
 
   auto txnID = nextRequestID_++;
@@ -846,7 +844,7 @@ HgImporter::TransactionID HgImporter::sendFetchTreeRequest(
     RelativePathPiece path,
     Hash pathManifestNode) {
 #if defined(EDEN_HAVE_STATS)
-  stats_->getStatsForCurrentThread().hgImporterFetchTree.addValue(1);
+  stats_->hgImporterFetchTree.addValue(1);
 #endif
 
   auto txnID = nextRequestID_++;
@@ -953,7 +951,7 @@ const ImporterOptions& HgImporter::getOptions() const {
 HgImporterManager::HgImporterManager(
     AbsolutePathPiece repoPath,
     LocalStore* store,
-    std::shared_ptr<EdenStats> stats,
+    std::shared_ptr<EdenThreadStats> stats,
     std::optional<AbsolutePath> importHelperScript)
     : repoPath_{repoPath},
       store_{store},

eden/fs/store/hg/HgImporter.h

@@ -120,10 +120,13 @@ class Importer {
  * code.  HgImporter hides all of the interaction with the underlying python
  * code.
  *
- * HgImporter is not thread safe.  The external caller must provide their own
- * locking around each HgImporter object.  However, to achieve parallelism
- * multiple HgImporter objects can be created for the same repository and used
- * simultaneously.
+ * HgImporter is thread-bound; use HgImporter only on the thread it was created
+ * on.  To achieve parallelism multiple HgImporter objects can be created for
+ * the same repository and used simultaneously.  HgImporter is thread-bound for
+ * the following reasons:
+ *
+ * * HgImporter does not synchronize its own members.
+ * * HgImporter accesses EdenThreadStats, and EdenThreadStats is thread-bound.
  */
 class HgImporter : public Importer {
  public:
@@ -137,7 +140,7 @@ class HgImporter : public Importer {
   HgImporter(
       AbsolutePathPiece repoPath,
       LocalStore* store,
-      std::shared_ptr<EdenStats>,
+      std::shared_ptr<EdenThreadStats>,
       std::optional<AbsolutePath> importHelperScript = std::nullopt);
 
   virtual ~HgImporter();
@@ -297,7 +300,7 @@ class HgImporter : public Importer {
 #endif
   const AbsolutePath repoPath_;
   LocalStore* const store_{nullptr};
-  std::shared_ptr<EdenStats> const stats_;
+  std::shared_ptr<EdenThreadStats> const stats_;
   ImporterOptions options_;
   uint32_t nextRequestID_{0};
   /**
@@ -330,13 +333,15 @@ class HgImporterError : public std::exception {
 /**
  * A helper class that manages an HgImporter and recreates it after any error
  * communicating with the underlying python hg_import_helper.py script.
+ *
+ * Because HgImporter is thread-bound, HgImporterManager is also thread-bound.
  */
 class HgImporterManager : public Importer {
  public:
   HgImporterManager(
       AbsolutePathPiece repoPath,
       LocalStore* store,
-      std::shared_ptr<EdenStats>,
+      std::shared_ptr<EdenThreadStats>,
       std::optional<AbsolutePath> importHelperScript = std::nullopt);
 
   Hash importFlatManifest(folly::StringPiece revName) override;
@@ -358,7 +363,7 @@ class HgImporterManager : public Importer {
 
   const AbsolutePath repoPath_;
   LocalStore* const store_{nullptr};
-  std::shared_ptr<EdenStats> const stats_;
+  std::shared_ptr<EdenThreadStats> const stats_;
   const std::optional<AbsolutePath> importHelperScript_;
 };
 

eden/fs/store/hg/test/HgBackingStoreTest.cpp

@@ -46,7 +46,7 @@ struct HgBackingStoreTest : TestRepo, ::testing::Test {
 
   std::shared_ptr<MemoryLocalStore> localStore{
       std::make_shared<MemoryLocalStore>()};
-  std::shared_ptr<EdenStats> stats{std::make_shared<EdenStats>()};
+  std::shared_ptr<EdenThreadStats> stats{std::make_shared<EdenThreadStats>()};
   HgImporter importer{repo.path(), localStore.get(), stats};
   std::shared_ptr<HgBackingStore> backingStore{
       std::make_shared<HgBackingStore>(&importer, localStore.get())};

eden/fs/store/hg/test/HgImportErrorsTest.cpp

@@ -181,7 +181,7 @@ class HgImportErrorTest : public ::testing::Test {
   std::shared_ptr<MemoryLocalStore> localStore_;
   std::shared_ptr<HgBackingStore> backingStore_;
   std::shared_ptr<ObjectStore> objectStore_;
-  std::shared_ptr<EdenStats> stats_ = std::make_shared<EdenStats>();
+  std::shared_ptr<EdenThreadStats> stats_ = std::make_shared<EdenThreadStats>();
 };
 
 AbsolutePath HgImportErrorTest::findFakeImportHelperPath() {

eden/fs/store/hg/test/HgImportTest.cpp

@@ -46,7 +46,7 @@ class HgImportTest : public ::testing::Test {
   AbsolutePath testPath_{testDir_.path().string()};
   HgRepo repo_{testPath_ + "repo"_pc};
   MemoryLocalStore localStore_;
-  std::shared_ptr<EdenStats> stats_ = std::make_shared<EdenStats>();
+  std::shared_ptr<EdenThreadStats> stats_ = std::make_shared<EdenThreadStats>();
 };
 
 } // namespace

eden/fs/store/hg/tester.cpp

@@ -202,7 +202,8 @@ int main(int argc, char* argv[]) {
 
   int returnCode = EX_OK;
   if (FLAGS_import_type == "flat") {
-    HgImporter importer(repoPath, &store, stats);
+    HgImporter importer(
+        repoPath, &store, getSharedStatsForCurrentThread(stats));
     printf("Importing revision \"%s\" using flat manifest\n", revName.c_str());
     auto rootHash = importer.importFlatManifest(revName);
     printf("Imported root tree: %s\n", rootHash.toString().c_str());

eden/fs/tracing/EdenStats.cpp

@@ -11,6 +11,7 @@
 
 #include <folly/container/Array.h>
 #include <chrono>
+#include <memory>
 
 #include "eden/fs/eden-config.h"
 
@@ -36,6 +37,12 @@ void EdenStats::aggregate() {
   }
 }
 
+std::shared_ptr<EdenThreadStats> getSharedStatsForCurrentThread(
+    std::shared_ptr<EdenStats> stats) {
+  return std::shared_ptr<EdenThreadStats>(
+      stats, &stats->getStatsForCurrentThread());
+}
+
 EdenThreadStats::EdenThreadStats() {}
 
 EdenThreadStats::Histogram EdenThreadStats::createHistogram(

eden/fs/tracing/EdenStats.h

@@ -10,6 +10,7 @@
 #pragma once
 
 #include <folly/ThreadLocal.h>
+#include <memory>
 #include "common/stats/ThreadLocalStats.h"
 #include "eden/fs/eden-config.h"
 
@@ -38,6 +39,9 @@ class EdenStats {
   folly::ThreadLocal<EdenThreadStats, ThreadLocalTag, void> threadLocalStats_;
 };
 
+std::shared_ptr<EdenThreadStats> getSharedStatsForCurrentThread(
+    std::shared_ptr<EdenStats>);
+
 /**
  * EdenThreadStats contains various thread-local stats structures.
  *