Skip to content

Commit

Permalink
backingstore: use nullptr for aux-not-found
Browse files Browse the repository at this point in the history
Summary:
Use nullptr intead of exceptions to represent the low level "not found" state for file and tree aux data. liubov-dmitrieva discovered through profiling that we spend a lot of CPU time on exceptions. In particular, we first try a local-only fetch, and if the aux data doesn't exist, we raise an exception and then enqueue the aux data to be fetched in allow-remote mode.

I changed the low level SaplingNativeBackingStore::get{Blob,Tree}AuxData to use nullptr to mean not-found, and then updated callers.

Note that I did not change the batch aux codepaths. Since these typically fetch remotely, "not found" results are not expected (so it is okay to keep using exceptions).

Reviewed By: liubov-dmitrieva

Differential Revision: D67545966

fbshipit-source-id: 73e8fa7a2584192faa13295870bff6c38eec0727
  • Loading branch information
muirdm authored and facebook-github-bot committed Jan 10, 2025
1 parent 06bd894 commit 134a06d
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 33 deletions.
30 changes: 19 additions & 11 deletions eden/fs/store/hg/SaplingBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1169,7 +1169,7 @@ SaplingBackingStore::getTreeAuxData(
ObjectFetchContext::ObjectType::TreeAuxData);

auto auxData = getLocalTreeAuxData(proxyHash);
if (auxData.hasValue()) {
if (auxData.hasValue() && auxData.value()) {
stats_->increment(&SaplingBackingStoreStats::fetchTreeAuxDataSuccess);
stats_->increment(&SaplingBackingStoreStats::fetchTreeAuxDataLocal);
return folly::makeSemiFuture(GetTreeAuxResult{
Expand Down Expand Up @@ -1241,10 +1241,14 @@ folly::Try<TreeAuxDataPtr> SaplingBackingStore::getLocalTreeAuxData(
using GetTreeAuxDataResult = folly::Try<TreeAuxDataPtr>;

if (auxData.hasValue()) {
return GetTreeAuxDataResult{
std::make_shared<TreeAuxDataPtr::element_type>(TreeAuxData{
Hash32{std::move(auxData.value()->digest_hash)},
auxData.value()->digest_size})};
if (auxData.value()) {
return GetTreeAuxDataResult{
std::make_shared<TreeAuxDataPtr::element_type>(TreeAuxData{
Hash32{std::move(auxData.value()->digest_hash)},
auxData.value()->digest_size})};
} else {
return GetTreeAuxDataResult{nullptr};
}
} else {
return GetTreeAuxDataResult{auxData.exception()};
}
Expand Down Expand Up @@ -1495,7 +1499,7 @@ SaplingBackingStore::getBlobAuxData(
ObjectFetchContext::ObjectType::BlobAuxData);

auto auxData = getLocalBlobAuxData(proxyHash);
if (auxData.hasValue()) {
if (auxData.hasValue() && auxData.value()) {
stats_->increment(&SaplingBackingStoreStats::fetchBlobAuxDataSuccess);
stats_->increment(&SaplingBackingStoreStats::fetchBlobAuxDataLocal);
return folly::makeSemiFuture(GetBlobAuxResult{
Expand Down Expand Up @@ -1572,11 +1576,15 @@ folly::Try<BlobAuxDataPtr> SaplingBackingStore::getLocalBlobAuxData(
using GetBlobAuxDataResult = folly::Try<BlobAuxDataPtr>;

if (auxData.hasValue()) {
return GetBlobAuxDataResult{
std::make_shared<BlobAuxDataPtr::element_type>(BlobAuxData{
Hash20{std::move(auxData.value()->content_sha1)},
Hash32{std::move(auxData.value()->content_blake3)},
auxData.value()->total_size})};
if (auxData.value()) {
return GetBlobAuxDataResult{
std::make_shared<BlobAuxDataPtr::element_type>(BlobAuxData{
Hash20{std::move(auxData.value()->content_sha1)},
Hash32{std::move(auxData.value()->content_blake3)},
auxData.value()->total_size})};
} else {
return GetBlobAuxDataResult{nullptr};
}
} else {
return GetBlobAuxDataResult{auxData.exception()};
}
Expand Down
12 changes: 2 additions & 10 deletions eden/scm/lib/backingstore/src/SaplingNativeBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,14 +118,10 @@ SaplingNativeBackingStore::getTreeAuxData(NodeId node, bool local) {
"Importing tree aux data node={} from hgcache",
folly::hexlify(node));
return folly::makeTryWith([&] {
auto auxData = sapling_backingstore_get_tree_aux(
return sapling_backingstore_get_tree_aux(
*store_.get(),
rust::Slice<const uint8_t>{node.data(), node.size()},
fetch_mode);
XCHECK(
auxData.get(),
"sapling_backingstore_get_tree_aux returned a nullptr, but did not throw an exception.");
return auxData;
});
}

Expand Down Expand Up @@ -220,14 +216,10 @@ SaplingNativeBackingStore::getBlobAuxData(NodeId node, bool local) {
"Importing blob aux data node={} from hgcache",
folly::hexlify(node));
return folly::makeTryWith([&] {
auto auxData = sapling_backingstore_get_file_aux(
return sapling_backingstore_get_file_aux(
*store_.get(),
rust::Slice<const uint8_t>{node.data(), node.size()},
fetch_mode);
XCHECK(
auxData.get(),
"sapling_backingstore_get_file_aux returned a nullptr, but did not throw an exception.");
return auxData;
});
}

Expand Down
20 changes: 8 additions & 12 deletions eden/scm/lib/backingstore/src/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,12 +303,10 @@ pub fn sapling_backingstore_get_tree_aux(
node: &[u8],
fetch_mode: ffi::FetchMode,
) -> Result<SharedPtr<ffi::TreeAuxData>> {
Ok(SharedPtr::new(
store
.get_tree_aux(node, FetchMode::from(fetch_mode))
.and_then(|opt| opt.ok_or_else(|| Error::msg("no tree aux data found")))?
.into(),
))
match store.get_tree_aux(node, FetchMode::from(fetch_mode))? {
Some(aux) => Ok(SharedPtr::new(aux.into())),
None => Ok(SharedPtr::null()),
}
}

pub fn sapling_backingstore_get_tree_aux_batch(
Expand Down Expand Up @@ -373,12 +371,10 @@ pub fn sapling_backingstore_get_file_aux(
node: &[u8],
fetch_mode: ffi::FetchMode,
) -> Result<SharedPtr<ffi::FileAuxData>> {
Ok(SharedPtr::new(
store
.get_file_aux(node, FetchMode::from(fetch_mode))
.and_then(|opt| opt.ok_or_else(|| Error::msg("no file aux data found")))?
.into(),
))
match store.get_file_aux(node, FetchMode::from(fetch_mode))? {
Some(aux) => Ok(SharedPtr::new(aux.into())),
None => Ok(SharedPtr::null()),
}
}

pub fn sapling_backingstore_get_file_aux_batch(
Expand Down

0 comments on commit 134a06d

Please sign in to comment.