Skip to content

[llvm][cas] Fan out persistent storage to reduce filesystem load #11101

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

[llvm][cas] Fan out persistent storage to reduce filesystem load #11101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/include/llvm/CAS/OnDiskGraphDB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ class OnDiskGraphDB {
static ObjectID getExternalReference(const IndexProxy &I);

void getStandalonePath(StringRef FileSuffix, const IndexProxy &I,
SmallVectorImpl<char> &Path) const;
SmallVectorImpl<char> &PersistentPath,
SmallVectorImpl<char> *TempPath = nullptr) const;

ArrayRef<uint8_t> getDigest(InternalRef Ref) const;
ArrayRef<uint8_t> getDigest(const IndexProxy &I) const;
Expand Down
81 changes: 60 additions & 21 deletions llvm/lib/CAS/OnDiskGraphDB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
#include "llvm/CAS/OnDiskGraphDB.h"
#include "OnDiskCommon.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/StableHashing.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/CAS/OnDiskCASLogger.h"
#include "llvm/CAS/OnDiskHashMappedTrie.h"
Expand Down Expand Up @@ -596,7 +597,17 @@ Error OnDiskGraphDB::TempFile::keep(const Twine &Name) {
assert(!Done);
Done = true;
// Always try to close and rename.
std::error_code RenameEC = sys::fs::rename(TmpName, Name);
std::error_code RenameEC = sys::fs::create_directories(
sys::path::parent_path(Name.str())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should attempt the rename first and only create the directory on failure. That will increase the cost the first time we put a file in that subdirectory (+1 rename call), but decrease it for any subsequent files (-1 mkdir). A couple of small-ish CAS databases I have sitting around have 3000+ standalone files, which would be more than 2 per sub-directory on average. It's not a big deal either way since we can change this without a format change in the future.

);

// We consider both the creation of the directory for the persistent storage
// and the final rename itself to be a failure to rename. This is consistent
// with other filesystem APIs that provide file manipulation APIs that will
// automatically construct directories as needed, so it seems like
// a reasonable choice.
if (!RenameEC)
RenameEC = sys::fs::rename(TmpName, Name);

if (Logger)
Logger->log_TempFile_keep(TmpName, Name.str(), RenameEC);
Expand All @@ -607,6 +618,7 @@ Error OnDiskGraphDB::TempFile::keep(const Twine &Name) {
sys::fs::file_t File = sys::fs::convertFDToNativeFile(FD);
if (std::error_code EC = sys::fs::closeFile(File))
return errorCodeToError(EC);

FD = -1;

return errorCodeToError(RenameEC);
Expand Down Expand Up @@ -935,19 +947,22 @@ Error OnDiskGraphDB::validate(bool Deep, HashingFuncT Hasher) const {
case TrieRecord::StorageKind::Standalone:
case TrieRecord::StorageKind::StandaloneLeaf:
case TrieRecord::StorageKind::StandaloneLeaf0:
SmallString<256> Path;
getStandalonePath(TrieRecord::getStandaloneFileSuffix(D.SK), I, Path);
SmallString<256> PersistentPath;
getStandalonePath(TrieRecord::getStandaloneFileSuffix(D.SK), I,
PersistentPath);
// If need to validate the content of the file later, just load the
// buffer here. Otherwise, just check the existance of the file.
if (Deep) {
auto File = MemoryBuffer::getFile(Path, /*IsText=*/false,
auto File = MemoryBuffer::getFile(PersistentPath, /*IsText=*/false,
/*RequiresNullTerminator=*/false);
if (!File || !*File)
return formatError("record file \'" + Path + "\' does not exist");
return formatError("record file \'" + PersistentPath +
"\' does not exist");

FileBuffer = std::move(*File);
} else if (!llvm::sys::fs::exists(Path))
return formatError("record file \'" + Path + "\' does not exist");
} else if (!llvm::sys::fs::exists(PersistentPath))
return formatError("record file \'" + PersistentPath +
"\' does not exist");
}

if (!Deep)
Expand Down Expand Up @@ -1203,10 +1218,11 @@ OnDiskGraphDB::load(ObjectID ExternalRef) {
// Note: Creation logic guarantees that data that needs null-termination is
// suitably 0-padded. Requiring null-termination here would be too expensive
// for extremely large objects that happen to be page-aligned.
SmallString<256> Path;
getStandalonePath(TrieRecord::getStandaloneFileSuffix(Object.SK), I, Path);
SmallString<256> PersistentPath;
getStandalonePath(TrieRecord::getStandaloneFileSuffix(Object.SK), I,
PersistentPath);
ErrorOr<std::unique_ptr<MemoryBuffer>> OwnedBuffer = MemoryBuffer::getFile(
Path, /*IsText=*/false, /*RequiresNullTerminator=*/false);
PersistentPath, /*IsText=*/false, /*RequiresNullTerminator=*/false);
if (!OwnedBuffer)
return createCorruptObjectError(getDigest(I));

Expand Down Expand Up @@ -1249,9 +1265,29 @@ InternalRef OnDiskGraphDB::makeInternalRef(FileOffset IndexOffset) {
}

void OnDiskGraphDB::getStandalonePath(StringRef Suffix, const IndexProxy &I,
SmallVectorImpl<char> &Path) const {
Path.assign(RootPath.begin(), RootPath.end());
sys::path::append(Path, FilePrefix + Twine(I.Offset.get()) + Suffix);
SmallVectorImpl<char> &PersistentPath,
SmallVectorImpl<char> *TempPath) const {
PersistentPath.assign(RootPath.begin(), RootPath.end());
if (TempPath)
TempPath->assign(RootPath.begin(), RootPath.end());
SmallVector<char, 256> FileNameBuffer;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This buffer is a bit irksome, but the lack of a Twine iterator requires the copy in order to hash the filename.

We could just hash the Offset which would save the copy but given we have prefix and suffix available it seemed reasonable to use them.

I may look into the stable hash interfaces over the weekend to see if this can be performed in a better way.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just hash the offset. The prefix is the same for every file in the CAS and each offset only corresponds to a single file, so suffix is redundant with the offset for hashing purposes.

StringRef FileName =
(FilePrefix + Twine(I.Offset.get()) + Suffix).toStringRef(FileNameBuffer);

unsigned FileNameHash = stable_hash_name(FileName);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, stable_hash_... is not stable enough for our purposes. It is documented to be stable across environments and program executions, but critically it is allowed to change in future versions, which would require a CAS format version bump.


// This is around 10 bits of entropy given we're using radix-36
static const unsigned IntermediateDirNameLength = 2;
static const char Radix36Chars[] = "0123456789abcdefghijklmnopqrstuvwxyz";
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benlangmuir This is a relatively common encoding for people to use in this kind of context but I couldn't find an existing impl anywhere in the tree, and I was not sure that it was so incredibly such that it warranted adding it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I've usually seen this done in llvm:

toString(llvm::APInt(64, Hash), 36, /*Signed=*/false);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops sent before finishing this comment:

There is also std::to_chars, which doesn't need to allocate a string.

static_assert(sizeof(Radix36Chars) == 36 + /* null */ 1);
SmallString<IntermediateDirNameLength> IntermediateDirName;
for (unsigned Ch = 0; Ch < IntermediateDirNameLength; ++Ch) {
IntermediateDirName.push_back(Radix36Chars[FileNameHash % 36]);
FileNameHash /= 36;
}
sys::path::append(PersistentPath, IntermediateDirName, FileName);
if (TempPath)
sys::path::append(*TempPath, FileName);
}

OnDiskContent OnDiskGraphDB::getContentFromHandle(ObjectHandle OH) const {
Expand Down Expand Up @@ -1339,21 +1375,23 @@ Error OnDiskGraphDB::createStandaloneLeaf(IndexProxy &I, ArrayRef<char> Data) {
TrieRecord::StorageKind SK = Leaf0 ? TrieRecord::StorageKind::StandaloneLeaf0
: TrieRecord::StorageKind::StandaloneLeaf;

SmallString<256> Path;
SmallString<256> PersistentPath;
SmallString<256> TempPath;
int64_t FileSize = Data.size() + Leaf0;
getStandalonePath(TrieRecord::getStandaloneFileSuffix(SK), I, Path);
getStandalonePath(TrieRecord::getStandaloneFileSuffix(SK), I, PersistentPath,
&TempPath);

// Write the file. Don't reuse this mapped_file_region, which is read/write.
// Let load() pull up one that's read-only.
Expected<MappedTempFile> File = createTempFile(Path, FileSize);
Expected<MappedTempFile> File = createTempFile(TempPath, FileSize);
if (!File)
return File.takeError();
assert(File->size() == (uint64_t)FileSize);
llvm::copy(Data, File->data());
if (Leaf0)
File->data()[Data.size()] = 0;
assert(File->data()[Data.size()] == 0);
if (Error E = File->keep(Path))
if (Error E = File->keep(PersistentPath))
return E;

// Store the object reference.
Expand Down Expand Up @@ -1402,14 +1440,15 @@ Error OnDiskGraphDB::store(ObjectID ID, ArrayRef<ObjectID> Refs,
// Compute the storage kind, allocate it, and create the record.
TrieRecord::StorageKind SK = TrieRecord::StorageKind::Unknown;
FileOffset PoolOffset;
SmallString<256> Path;
SmallString<256> PersistentPath;
SmallString<256> TempPath;
std::optional<MappedTempFile> File;
std::optional<uint64_t> FileSize;
auto AllocStandaloneFile = [&](size_t Size) -> Expected<char *> {
getStandalonePath(TrieRecord::getStandaloneFileSuffix(
TrieRecord::StorageKind::Standalone),
I, Path);
if (Error E = createTempFile(Path, Size).moveInto(File))
I, PersistentPath, &TempPath);
if (Error E = createTempFile(TempPath, Size).moveInto(File))
return std::move(E);
assert(File->size() == Size);
FileSize = Size;
Expand Down Expand Up @@ -1463,7 +1502,7 @@ Error OnDiskGraphDB::store(ObjectID ID, ArrayRef<ObjectID> Refs,
if (File) {
if (Existing.SK == TrieRecord::StorageKind::Unknown) {
// Keep the file!
if (Error E = File->keep(Path))
if (Error E = File->keep(PersistentPath))
return E;
} else {
File.reset();
Expand Down