facebook/sapling
1
1
Fork 0
mirror of https://github.com/facebook/sapling.git synced 2024-10-08 15:57:43 +03:00
Code Issues Projects Releases Wiki Activity

eden: make HgImporter safer

Browse Source 
Summary:
We want to have a shared pool of `HgImporter` across different threads, but that would require `HgImporter` is safe to be *passed* between threads. (This is not making `HgImporter` entirely thread safe.)

However, `HgImporter` currently holds a pointer to the thread local fb303 counter. This diff pushes down `EdenStats` so we only access the thread local stats variable when we need to add counters. This might be a little slower but it does make it safer.

Reviewed By: chadaustin

Differential Revision: D19053250

fbshipit-source-id: 44a897acc90c6042ae22a0417eece39e099ee13f
This commit is contained in:
Zeyi (Rice) Fan 2020-01-16 12:43:54 -08:00 committed by Facebook Github Bot
parent bd7b3a38c8
commit 59c2cb7aff
5 changed files with 16 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
eden/fs/store/hg/HgBackingStore.cpp
View File

@ -112,8 +112,7 @@ 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_, getSharedHgImporterStatsForCurrentThread(stats_)));
threadLocalImporter.reset(new HgImporterManager(repository_, stats_));
func();
});
}
@ -224,8 +223,7 @@ HgBackingStore::HgBackingStore(
XLOG(WARN) << "Rust native store is disabled due to: " << ex.what();
}
#endif
HgImporter importer(
repository, getSharedHgImporterStatsForCurrentThread(stats));
HgImporter importer(repository, stats);
const auto& options = importer.getOptions();
initializeTreeManifestImport(options, repository);
repoName_ = options.repoName;

15
eden/fs/store/hg/HgImporter.cpp
View File

@ -129,7 +129,7 @@ class HgImporterEofError : public HgImporterError {
HgImporter::HgImporter(
AbsolutePathPiece repoPath,
std::shared_ptr<HgImporterThreadStats> stats,
std::shared_ptr<EdenStats> stats,
std::optional<AbsolutePath> importHelperScript)
: repoPath_{repoPath}, stats_{std::move(stats)} {
std::vector<string> cmd;
@ -484,7 +484,7 @@ HgImporter::ChunkHeader HgImporter::readChunkHeader(
HgImporter::TransactionID HgImporter::sendManifestRequest(
folly::StringPiece revName) {
stats_->manifest.addValue(1);
stats_->getHgImporterStatsForCurrentThread().manifest.addValue(1);
auto txnID = nextRequestID_++;
ChunkHeader header;
@ -505,7 +505,8 @@ HgImporter::TransactionID HgImporter::sendManifestRequest(
HgImporter::TransactionID HgImporter::sendManifestNodeRequest(
folly::StringPiece revName) {
stats_->manifestNodeForCommit.addValue(1);
stats_->getHgImporterStatsForCurrentThread().manifestNodeForCommit.addValue(
1);
auto txnID = nextRequestID_++;
ChunkHeader header;
@ -527,7 +528,7 @@ HgImporter::TransactionID HgImporter::sendManifestNodeRequest(
HgImporter::TransactionID HgImporter::sendFileRequest(
RelativePathPiece path,
Hash revHash) {
stats_->catFile.addValue(1);
stats_->getHgImporterStatsForCurrentThread().catFile.addValue(1);
auto txnID = nextRequestID_++;
ChunkHeader header;
@ -551,7 +552,7 @@ HgImporter::TransactionID HgImporter::sendFileRequest(
HgImporter::TransactionID HgImporter::sendPrefetchFilesRequest(
const std::vector<std::pair<RelativePath, Hash>>& files) {
stats_->prefetchFiles.addValue(1);
stats_->getHgImporterStatsForCurrentThread().prefetchFiles.addValue(1);
auto txnID = nextRequestID_++;
ChunkHeader header;
@ -608,7 +609,7 @@ HgImporter::TransactionID HgImporter::sendPrefetchFilesRequest(
HgImporter::TransactionID HgImporter::sendFetchTreeRequest(
RelativePathPiece path,
Hash pathManifestNode) {
stats_->fetchTree.addValue(1);
stats_->getHgImporterStatsForCurrentThread().fetchTree.addValue(1);
auto txnID = nextRequestID_++;
ChunkHeader header;
@ -713,7 +714,7 @@ const ImporterOptions& HgImporter::getOptions() const {
HgImporterManager::HgImporterManager(
AbsolutePathPiece repoPath,
std::shared_ptr<HgImporterThreadStats> stats,
std::shared_ptr<EdenStats> stats,
std::optional<AbsolutePath> importHelperScript)
: repoPath_{repoPath},
stats_{std::move(stats)},

8
eden/fs/store/hg/HgImporter.h
View File

@ -127,7 +127,7 @@ class HgImporter : public Importer {
*/
HgImporter(
AbsolutePathPiece repoPath,
std::shared_ptr<HgImporterThreadStats>,
std::shared_ptr<EdenStats>,
std::optional<AbsolutePath> importHelperScript = std::nullopt);
virtual ~HgImporter();
@ -276,7 +276,7 @@ class HgImporter : public Importer {
facebook::eden::Subprocess helper_;
#endif
const AbsolutePath repoPath_;
std::shared_ptr<HgImporterThreadStats> const stats_;
std::shared_ptr<EdenStats> const stats_;
ImporterOptions options_;
uint32_t nextRequestID_{0};
/**
@ -316,7 +316,7 @@ class HgImporterManager : public Importer {
public:
HgImporterManager(
AbsolutePathPiece repoPath,
std::shared_ptr<HgImporterThreadStats>,
std::shared_ptr<EdenStats>,
std::optional<AbsolutePath> importHelperScript = std::nullopt);
Hash resolveManifestNode(folly::StringPiece revName) override;
@ -338,7 +338,7 @@ class HgImporterManager : public Importer {
std::unique_ptr<HgImporter> importer_;
const AbsolutePath repoPath_;
std::shared_ptr<HgImporterThreadStats> const stats_;
std::shared_ptr<EdenStats> const stats_;
const std::optional<AbsolutePath> importHelperScript_;
};

3
eden/fs/store/hg/test/HgBackingStoreTest.cpp
View File

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

3
eden/fs/store/hg/test/HgImportTest.cpp
View File

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