Skip to content

Commit

Permalink
Plumb prefix/searchRoot through to edenAPI globFiles
Browse files Browse the repository at this point in the history
Summary:
# Context
Returning only results that match a prefix is a feature supported by the suffix_query api. Previously we were not planning to support this feature, but now allow it for limited prefixes.

This allows us to greatly increase the speed of running the suffix query api

# This Diff
Plumb prefix through the suffix_query call path. Suffix query is equal to the searchRoot.

# Technical Details
Since we've moved www into fbsource, this query hits a wider range of intended results than when it was run inside www. Returning all these results is slow, so by cutting them out on the server side, we can greatly increase the overall speed

On the rust side, it's an optional<Vec<String>>, but since the cxx ffi doesn't support passing optionals through, we use an empty list and check on the rust side.

Reviewed By: muirdm

Differential Revision: D66501881

fbshipit-source-id: 7c87f9233aeca9d861c045769c9f4e7a8eaec145
  • Loading branch information
Chris Dinh authored and facebook-github-bot committed Nov 26, 2024
1 parent c6b20ec commit a49dd0b
Show file tree
Hide file tree
Showing 23 changed files with 124 additions and 62 deletions.
24 changes: 18 additions & 6 deletions eden/fs/service/EdenServiceHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3794,24 +3794,40 @@ EdenServiceHandler::semifuture_globFiles(std::unique_ptr<GlobParams> params) {
globFilesRequestScope->setLocal(false);
// Attempt to resolve all EdenAPI futures. If any of
// them result in an error we will fall back to local lookup

auto searchRoot = params->searchRoot_ref().value();
size_t pos = 0;
while ((pos = searchRoot.find("\\", pos)) != std::string::npos) {
searchRoot.replace(pos, 1, "/");
}

auto combinedFuture =
std::move(backgroundFuture)
.thenValue([revisions = params->revisions_ref().value(),
mountHandle,
suffixGlobs = std::move(suffixGlobs),
searchRoot,
serverState = server_->getServerState(),
includeDotfiles = *params->includeDotfiles(),
context = context.copy()](auto&&) mutable {
auto& store = mountHandle.getObjectStore();
const auto& edenMount = mountHandle.getEdenMountPtr();
const auto& rootInode = mountHandle.getRootInode();

std::vector<std::string> prefixes;
// Despite the API supporting multiple prefixes, we only use one
// derived from the search root
if (!searchRoot.empty() && searchRoot != ".") {
prefixes.push_back(searchRoot);
}

if (revisions.empty()) {
return getLocalGlobResults(
edenMount,
serverState,
includeDotfiles,
suffixGlobs,
prefixes,
rootInode,
context);
}
Expand All @@ -3822,7 +3838,7 @@ EdenServiceHandler::semifuture_globFiles(std::unique_ptr<GlobParams> params) {
// text version globFiles takes as input the human readable
// version, so convert using the store's parse method
globFilesResultFutures.push_back(store.getGlobFiles(
store.parseRootId(id), suffixGlobs, context));
store.parseRootId(id), suffixGlobs, prefixes, context));
}
return collectAllSafe(std::move(globFilesResultFutures));
});
Expand All @@ -3834,7 +3850,7 @@ EdenServiceHandler::semifuture_globFiles(std::unique_ptr<GlobParams> params) {
wantDtype = params->wantDtype_ref().value(),
includeDotfiles =
params->includeDotfiles_ref().value(),
searchRoot = params->searchRoot_ref().value(),
searchRoot,
&context](auto&& globResults) mutable {
auto edenMount = mountHandle.getEdenMountPtr();
std::vector<ImmediateFuture<GlobEntry>> globEntryFuts;
Expand Down Expand Up @@ -3938,10 +3954,6 @@ EdenServiceHandler::semifuture_globFiles(std::unique_ptr<GlobParams> params) {
// Windows
XLOG(DBG5)
<< "Building Glob with searchroot " << searchRoot;
size_t pos = searchRoot.find("\\");
if (pos != std::string::npos) {
searchRoot.replace(pos, 1, "/");
}
auto glob = std::make_unique<Glob>();
std::sort(
globEntries.begin(),
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/service/ThriftGlobImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -367,13 +367,14 @@ getLocalGlobResults(
const std::shared_ptr<ServerState>& serverState,
bool includeDotfiles,
const std::vector<std::string>& suffixGlobs,
const std::vector<std::string>& prefixes,
const TreeInodePtr& rootInode,
const ObjectFetchContextPtr& context) {
// Use current commit hash
XLOG(DBG3) << "No commit hash in input, using current hash";
auto rootId = edenMount->getCheckedOutRootId();
auto& store = edenMount->getObjectStore();
return store->getGlobFiles(rootId, suffixGlobs, context)
return store->getGlobFiles(rootId, suffixGlobs, prefixes, context)
.thenValue([edenMount,
serverState,
includeDotfiles,
Expand Down
1 change: 1 addition & 0 deletions eden/fs/service/ThriftGlobImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ getLocalGlobResults(
const std::shared_ptr<ServerState>& serverState,
bool includeDotfiles,
const std::vector<std::string>& suffixGlobs,
const std::vector<std::string>& prefixes,
const TreeInodePtr& rootInode,
const ObjectFetchContextPtr& context);

Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/BackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,8 @@ class BackingStore : public RootIdCodec, public ObjectIdCodec {
*/
virtual ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) = 0;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) = 0;

/**
* Prefetch all the blobs represented by the HashRange.
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/EmptyBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ SemiFuture<BackingStore::GetBlobAuxResult> EmptyBackingStore::getBlobAuxData(
ImmediateFuture<BackingStore::GetGlobFilesResult>
EmptyBackingStore::getGlobFiles(
const RootId& /* id */,
const std::vector<std::string>& /* globs */) {
const std::vector<std::string>& /* globs */,
const std::vector<std::string>& /* prefixes */) {
return makeSemiFuture<GetGlobFilesResult>(
std::domain_error("empty backing store"));
}
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/EmptyBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ class EmptyBackingStore final : public BijectiveBackingStore {

ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) override;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) override;

LocalStoreCachingPolicy localStoreCachingPolicy_ =
LocalStoreCachingPolicy::NoCaching;
Expand Down
5 changes: 3 additions & 2 deletions eden/fs/store/FilteredBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -392,9 +392,10 @@ folly::SemiFuture<folly::Unit> FilteredBackingStore::prefetchBlobs(
ImmediateFuture<BackingStore::GetGlobFilesResult>
FilteredBackingStore::getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) {
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) {
auto [parsedRootId, parsedFilterId] = parseFilterIdFromRootId(id);
auto fut = backingStore_->getGlobFiles(parsedRootId, globs);
auto fut = backingStore_->getGlobFiles(parsedRootId, globs, prefixes);
return std::move(fut).thenValue([this, id, filterId = parsedFilterId](
auto&& getGlobFilesResult) {
std::vector<ImmediateFuture<std::pair<std::string, FilterCoverage>>>
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/FilteredBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ class FilteredBackingStore

ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) override;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) override;

/*
* Does the actual filtering logic for tree and root-tree objects.
Expand Down
8 changes: 4 additions & 4 deletions eden/fs/store/ObjectStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -782,19 +782,19 @@ ImmediateFuture<bool> ObjectStore::areBlobsEqual(
}

ImmediateFuture<BackingStore::GetGlobFilesResult> ObjectStore::getGlobFiles(

const RootId& id,
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes,
const ObjectFetchContextPtr& context) const {
return getGlobFilesImpl(id, globs, context);
return getGlobFilesImpl(id, globs, prefixes, context);
}

ImmediateFuture<BackingStore::GetGlobFilesResult> ObjectStore::getGlobFilesImpl(

const RootId& id,
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes,
const ObjectFetchContextPtr& /*context*/) const {
return backingStore_->getGlobFiles(id, globs);
return backingStore_->getGlobFiles(id, globs, prefixes);
}

ObjectComparison ObjectStore::compareObjectsById(
Expand Down
2 changes: 2 additions & 0 deletions eden/fs/store/ObjectStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ class ObjectStore : public IObjectStore,
ImmediateFuture<BackingStore::GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes,
const ObjectFetchContextPtr& context) const;

/**
Expand Down Expand Up @@ -393,6 +394,7 @@ class ObjectStore : public IObjectStore,
ImmediateFuture<BackingStore::GetGlobFilesResult> getGlobFilesImpl(
const RootId& id,
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes,
const ObjectFetchContextPtr& context) const;

/**
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/git/GitBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,8 @@ GitBackingStore::getBlobAuxData(const ObjectId&, const ObjectFetchContextPtr&) {

ImmediateFuture<BackingStore::GetGlobFilesResult> GitBackingStore::getGlobFiles(
const RootId& /* id */,
const std::vector<std::string>& /* globs */) {
const std::vector<std::string>& /* globs */,
const std::vector<std::string>& /* prefixes */) {
return folly::makeFuture<GetGlobFilesResult>(
std::runtime_error("getGlobFiles() is not supported on git"));
};
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/git/GitBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ class GitBackingStore final : public BijectiveBackingStore {
const ObjectFetchContextPtr& context) override;
ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) override;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) override;

TreePtr getTreeImpl(const ObjectId& id);
BlobPtr getBlobImpl(const ObjectId& id);
Expand Down
5 changes: 3 additions & 2 deletions eden/fs/store/hg/SaplingBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2288,10 +2288,11 @@ folly::SemiFuture<folly::Unit> SaplingBackingStore::prefetchBlobs(
ImmediateFuture<BackingStore::GetGlobFilesResult>
SaplingBackingStore::getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) {
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) {
folly::stop_watch<std::chrono::milliseconds> watch;
using GetGlobFilesResult = folly::Try<GetGlobFilesResult>;
auto globFilesResult = store_.getGlobFiles(id.value(), globs);
auto globFilesResult = store_.getGlobFiles(id.value(), globs, prefixes);

if (globFilesResult.hasValue()) {
std::vector<std::string> files;
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/hg/SaplingBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,8 @@ class SaplingBackingStore final : public BackingStore {

ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) override;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) override;

/**
* The worker runloop function.
Expand Down
20 changes: 12 additions & 8 deletions eden/fs/store/hg/test/SaplingBackingStoreTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,9 @@ TEST_F(SaplingBackingStoreWithFaultInjectorTest, getBlob) {

TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesMultiple) {
auto suffixes = std::vector<std::string>{".txt"};
auto globFiles =
queuedBackingStore->getGlobFiles(commit1, suffixes).get(kTestTimeout);
auto prefixes = std::vector<std::string>{};
auto globFiles = queuedBackingStore->getGlobFiles(commit1, suffixes, prefixes)
.get(kTestTimeout);
auto paths = globFiles.globFiles;
auto commitId = queuedBackingStore->renderRootId(globFiles.rootId);

Expand All @@ -229,8 +230,9 @@ TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesMultiple) {

TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesSingle) {
auto suffixes = std::vector<std::string>{".rs"};
auto globFiles =
queuedBackingStore->getGlobFiles(commit1, suffixes).get(kTestTimeout);
auto prefixes = std::vector<std::string>{};
auto globFiles = queuedBackingStore->getGlobFiles(commit1, suffixes, prefixes)
.get(kTestTimeout);
auto paths = globFiles.globFiles;
auto commitId = queuedBackingStore->renderRootId(globFiles.rootId);

Expand All @@ -246,8 +248,9 @@ TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesSingle) {
}
TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesNone) {
auto suffixes = std::vector<std::string>{".bzl"};
auto globFiles =
queuedBackingStore->getGlobFiles(commit1, suffixes).get(kTestTimeout);
auto prefixes = std::vector<std::string>{};
auto globFiles = queuedBackingStore->getGlobFiles(commit1, suffixes, prefixes)
.get(kTestTimeout);
auto paths = globFiles.globFiles;
auto commitId = queuedBackingStore->renderRootId(globFiles.rootId);

Expand All @@ -261,8 +264,9 @@ TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesNone) {

TEST_F(SaplingBackingStoreNoFaultInjectorTest, getGlobFilesNested) {
auto suffixes = std::vector<std::string>{".cpp"};
auto globFiles =
queuedBackingStore->getGlobFiles(commit1, suffixes).get(kTestTimeout);
auto prefixes = std::vector<std::string>{};
auto globFiles = queuedBackingStore->getGlobFiles(commit1, suffixes, prefixes)
.get(kTestTimeout);
auto paths = globFiles.globFiles;
auto commitId = queuedBackingStore->renderRootId(globFiles.rootId);

Expand Down
55 changes: 35 additions & 20 deletions eden/fs/store/test/FilteredBackingStoreTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -834,26 +834,41 @@ TEST_F(FakeSubstringFilteredBackingStoreTest, getGlobFiles) {
// Get the glob files
auto executor = folly::ManualExecutor();

auto filteredFut1 =
filteredStore_->getGlobFiles(rootId, std::vector<std::string>{"foo"})
.semi()
.via(&executor);
auto filteredFut2 =
filteredStore_->getGlobFiles(rootId2, std::vector<std::string>{"foo"})
.semi()
.via(&executor);
auto filteredFut3 =
filteredStore_->getGlobFiles(rootId3, std::vector<std::string>{"foo"})
.semi()
.via(&executor);
auto filteredFut4 =
filteredStore_->getGlobFiles(rootId4, std::vector<std::string>{"foo"})
.semi()
.via(&executor);
auto filteredFut5 =
filteredStore_->getGlobFiles(rootId5, std::vector<std::string>{"foo"})
.semi()
.via(&executor);
auto filteredFut1 = filteredStore_
->getGlobFiles(
rootId,
std::vector<std::string>{"foo"},
std::vector<std::string>{})
.semi()
.via(&executor);
auto filteredFut2 = filteredStore_
->getGlobFiles(
rootId2,
std::vector<std::string>{"foo"},
std::vector<std::string>{})
.semi()
.via(&executor);
auto filteredFut3 = filteredStore_
->getGlobFiles(
rootId3,
std::vector<std::string>{"foo"},
std::vector<std::string>{})
.semi()
.via(&executor);
auto filteredFut4 = filteredStore_
->getGlobFiles(
rootId4,
std::vector<std::string>{"foo"},
std::vector<std::string>{})
.semi()
.via(&executor);
auto filteredFut5 = filteredStore_
->getGlobFiles(
rootId5,
std::vector<std::string>{"foo"},
std::vector<std::string>{})
.semi()
.via(&executor);
executor.drain();
EXPECT_TRUE(filteredFut1.isReady());
EXPECT_TRUE(filteredFut2.isReady());
Expand Down
5 changes: 4 additions & 1 deletion eden/fs/store/test/ObjectStoreTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,10 @@ TEST_F(ObjectStoreTest, glob_files_test) {
auto globs = std::vector<std::string>{".txt"};

auto fut = objectStore->getGlobFiles(
rootId, globs, context.as<ObjectFetchContext>());
rootId,
globs,
std::vector<std::string>{},
context.as<ObjectFetchContext>());
auto result = std::move(fut).get(0ms);
EXPECT_EQ(result.globFiles.size(), 2);
auto sorted_result = result.globFiles;
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/testharness/FakeBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ FakeBackingStore::getBlobAuxData(
ImmediateFuture<BackingStore::GetGlobFilesResult>
FakeBackingStore::getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) {
const std::vector<std::string>& globs,
const std::vector<std::string>& /*prefixes*/) {
// Since unordered map can't take a vec for testing purposes only use the
// first entry in the query
auto suffixQuery = std::pair<RootId, std::string>(id, globs[0]);
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/testharness/FakeBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ class FakeBackingStore final : public BackingStore {
const ObjectFetchContextPtr& context) override;
ImmediateFuture<GetGlobFilesResult> getGlobFiles(
const RootId& id,
const std::vector<std::string>& globs) override;
const std::vector<std::string>& globs,
const std::vector<std::string>& prefixes) override;

LocalStoreCachingPolicy localStoreCachingPolicy_;
std::shared_ptr<ServerState> serverState_;
Expand Down
7 changes: 4 additions & 3 deletions eden/scm/lib/backingstore/include/SaplingNativeBackingStore.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This software may be used and distributed according to the terms of the
* GNU General Public License version 2.
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#pragma once
Expand Down Expand Up @@ -118,7 +118,8 @@ class SaplingNativeBackingStore {

folly::Try<std::shared_ptr<GlobFilesResponse>> getGlobFiles(
std::string_view commit_id,
const std::vector<std::string>& suffixes);
const std::vector<std::string>& suffixes,
const std::vector<std::string>& prefixes);

void flush();

Expand Down
Loading

0 comments on commit a49dd0b

Please sign in to comment.