From a5f53c6e3af4a643b9b69da469cd55878eb2ca20 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Thu, 14 Jun 2018 21:32:45 -0700 Subject: [PATCH] avoid blocking on HgProxyHash::getBatch() in hg importer threads Summary: D8065370 changed the HgImporter code to make a blocking `get()` call on a future from the hg importer thread pool. This caused deadlocks, since all of the hg importer threads could become stuck waiting on these `get()` calls to complete. These would be waiting on RocksDbLocalStore threads which were in turn all busy waiting to schedule operations on the HgImporter threads. This fixes the code to use `Future::then()` rather than `Future::get()` to avoid blocking the HgImporter threads on these operations. Reviewed By: wez Differential Revision: D8438777 fbshipit-source-id: a0d647b10ef5a182be2d19f636c2dbc24eab1b23 --- eden/fs/store/BackingStore.h | 2 +- eden/fs/store/hg/HgBackingStore.cpp | 14 ++++++------- eden/fs/store/hg/HgBackingStore.h | 2 +- eden/fs/store/hg/HgImporter.cpp | 5 ++--- eden/fs/store/hg/HgImporter.h | 6 ++++-- eden/fs/store/hg/test/HgPrefetchTest.cpp | 25 +++++++++++++++--------- 6 files changed, 30 insertions(+), 24 deletions(-) diff --git a/eden/fs/store/BackingStore.h b/eden/fs/store/BackingStore.h index 51ab2bb017..623a8042d5 100644 --- a/eden/fs/store/BackingStore.h +++ b/eden/fs/store/BackingStore.h @@ -42,7 +42,7 @@ class BackingStore { virtual folly::Future> getBlob(const Hash& id) = 0; virtual folly::Future> getTreeForCommit( const Hash& commitID) = 0; - virtual folly::Future prefetchBlobs( + FOLLY_NODISCARD virtual folly::Future prefetchBlobs( const std::vector& ids) const { return folly::unit; } diff --git a/eden/fs/store/hg/HgBackingStore.cpp b/eden/fs/store/hg/HgBackingStore.cpp index db591a85a7..1e93769623 100644 --- a/eden/fs/store/hg/HgBackingStore.cpp +++ b/eden/fs/store/hg/HgBackingStore.cpp @@ -21,6 +21,7 @@ #include "eden/fs/store/LocalStore.h" #include "eden/fs/store/StoreResult.h" #include "eden/fs/store/hg/HgImporter.h" +#include "eden/fs/store/hg/HgProxyHash.h" #include "eden/fs/utils/UnboundedQueueThreadPool.h" using folly::ByteRange; @@ -153,14 +154,11 @@ Future> HgBackingStore::getBlob(const Hash& id) { folly::Future HgBackingStore::prefetchBlobs( const std::vector& ids) const { - return folly::via( - importThreadPool_.get(), - [ids] { - getThreadLocalImporter().prefetchFiles(ids); - return folly::unit; - }) - // Ensure that the control moves back to the main thread pool - // to process the caller-attached .then routine. + return HgProxyHash::getBatch(localStore_, ids) + .via(importThreadPool_.get()) + .then([](std::vector>&& hgPathHashes) { + return getThreadLocalImporter().prefetchFiles(hgPathHashes); + }) .via(serverThreadPool_); } diff --git a/eden/fs/store/hg/HgBackingStore.h b/eden/fs/store/hg/HgBackingStore.h index 06c9855918..70872fa4eb 100644 --- a/eden/fs/store/hg/HgBackingStore.h +++ b/eden/fs/store/hg/HgBackingStore.h @@ -54,7 +54,7 @@ class HgBackingStore : public BackingStore { folly::Future> getBlob(const Hash& id) override; folly::Future> getTreeForCommit( const Hash& commitID) override; - folly::Future prefetchBlobs( + FOLLY_NODISCARD folly::Future prefetchBlobs( const std::vector& ids) const override; private: diff --git a/eden/fs/store/hg/HgImporter.cpp b/eden/fs/store/hg/HgImporter.cpp index e6e7a76ee3..78cd7e5d04 100644 --- a/eden/fs/store/hg/HgImporter.cpp +++ b/eden/fs/store/hg/HgImporter.cpp @@ -623,9 +623,8 @@ IOBuf HgImporter::importFileContents(Hash blobHash) { return buf; } -void HgImporter::prefetchFiles(const std::vector& blobHashes) { - auto files = HgProxyHash::getBatch(store_, blobHashes).get(); - +void HgImporter::prefetchFiles( + const std::vector>& files) { sendPrefetchFilesRequest(files); // Read the response; throws if there was any error. diff --git a/eden/fs/store/hg/HgImporter.h b/eden/fs/store/hg/HgImporter.h index 7128537859..8c217a6aee 100644 --- a/eden/fs/store/hg/HgImporter.h +++ b/eden/fs/store/hg/HgImporter.h @@ -69,7 +69,8 @@ class Importer { */ virtual folly::IOBuf importFileContents(Hash blobHash) = 0; - virtual void prefetchFiles(const std::vector& blobHashes) = 0; + virtual void prefetchFiles( + const std::vector>& files) = 0; }; /** @@ -134,7 +135,8 @@ class HgImporter : public Importer { std::unique_ptr importTree(const Hash& id) override; folly::IOBuf importFileContents(Hash blobHash) override; - void prefetchFiles(const std::vector& blobHashes) override; + void prefetchFiles( + const std::vector>& files) override; /** * Resolve the manifest node for the specified revision. diff --git a/eden/fs/store/hg/test/HgPrefetchTest.cpp b/eden/fs/store/hg/test/HgPrefetchTest.cpp index 96f2d83739..712ec7085a 100644 --- a/eden/fs/store/hg/test/HgPrefetchTest.cpp +++ b/eden/fs/store/hg/test/HgPrefetchTest.cpp @@ -16,11 +16,13 @@ #include "eden/fs/model/Tree.h" #include "eden/fs/model/TreeEntry.h" #include "eden/fs/store/MemoryLocalStore.h" -#include "eden/fs/store/hg/HgImporter.h" +#include "eden/fs/store/hg/HgBackingStore.h" #include "eden/fs/testharness/HgRepo.h" #include "eden/fs/utils/PathFuncs.h" +#include "eden/fs/utils/UnboundedQueueThreadPool.h" using namespace facebook::eden; +using namespace std::chrono_literals; using folly::StringPiece; using folly::test::TemporaryDirectory; using testing::HasSubstr; @@ -169,14 +171,19 @@ usecunionstore=True EXPECT_THAT( catOutputs.second, HasSubstr("no remotefilelog server configured")); + // Build an HgBackingStore for this repository + UnboundedQueueThreadPool resultThreadPool(1, "ResultThread"); + HgBackingStore store(clientRepo.path(), &localStore, &resultThreadPool); + // Now test running prefetch // Build a list of file blob IDs to prefetch. - HgImporter importer(clientRepo.path(), &localStore); - auto rootTreeHash = importer.importManifest(commit2.toString()); - auto rootTree = importer.importTree(rootTreeHash); - auto srcTree = importer.importTree(rootTree->getEntryAt("src"_pc).getHash()); - auto edenTree = importer.importTree(srcTree->getEntryAt("eden"_pc).getHash()); - auto fooTree = importer.importTree(rootTree->getEntryAt("foo"_pc).getHash()); + auto rootTree = store.getTreeForCommit(commit2).get(10s); + auto srcTree = + store.getTree(rootTree->getEntryAt("src"_pc).getHash()).get(1s); + auto edenTree = + store.getTree(srcTree->getEntryAt("eden"_pc).getHash()).get(1s); + auto fooTree = + store.getTree(rootTree->getEntryAt("foo"_pc).getHash()).get(1s); std::vector blobHashes; blobHashes.push_back(edenTree->getEntryAt("main.py"_pc).getHash()); @@ -187,8 +194,8 @@ usecunionstore=True blobHashes.push_back(fooTree->getEntryAt("bar.txt"_pc).getHash()); blobHashes.push_back(fooTree->getEntryAt("test.txt"_pc).getHash()); - // Call prefetch - importer.prefetchFiles(blobHashes); + // Call prefetchBlobs() + store.prefetchBlobs(blobHashes).get(10s); // Running "hg cat" with ssh disabled and no server repo should succeed now // that we have prefetched the data.