diff --git a/cpp/backend/common/BUILD b/cpp/backend/common/BUILD index 2e1560d3f..d6c3b2b86 100644 --- a/cpp/backend/common/BUILD +++ b/cpp/backend/common/BUILD @@ -36,6 +36,11 @@ cc_library( deps = [ ":page", ":page_id", + "//common:status_util", + "//common:fstream", + "@com_google_absl//absl/status:status", + "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings:str_format", ], ) @@ -45,6 +50,7 @@ cc_test( deps = [ ":file", "//common:file_util", + "//common:status_test_util", "@com_google_googletest//:gtest_main", ], ) @@ -56,8 +62,10 @@ cc_binary( deps = [ ":file", "//common:file_util", - "//third_party/gperftools:profiler", + "//common:status_test_util", "@com_github_google_benchmark//:benchmark_main", + "@com_google_absl//absl/status:statusor", + "//third_party/gperftools:profiler", ], ) @@ -100,6 +108,9 @@ cc_library( ":file", ":page_id", "//common:memory_usage", + "//common:status_util", + "@com_google_absl//absl/status:status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/container:flat_hash_map", ], ) @@ -110,6 +121,7 @@ cc_test( deps = [ ":page", ":page_pool", + "//common:status_test_util", "@com_google_googletest//:gtest_main", ], ) @@ -121,9 +133,11 @@ cc_binary( ":access_pattern", ":eviction_policy", ":page_pool", + "//common:status_test_util", "//third_party/gperftools:profiler", "@com_github_google_benchmark//:benchmark_main", ], + testonly = True, ) cc_library( @@ -134,6 +148,7 @@ cc_library( ":page", ":page_id", ":page_pool", + "@com_google_absl//absl/status:statusor", ], ) @@ -144,5 +159,10 @@ cc_test( ":page_manager", "//common:status_test_util", "@com_google_googletest//:gtest_main", + "@com_github_google_benchmark//:benchmark_main", + "@com_google_absl//absl/status:status", + "@com_google_absl//absl/status:statusor", + "//third_party/gperftools:profiler", ], + testonly = True, ) diff --git a/cpp/backend/common/access_pattern.h b/cpp/backend/common/access_pattern.h index 3c400aea7..b94754811 100644 --- a/cpp/backend/common/access_pattern.h +++ b/cpp/backend/common/access_pattern.h @@ -24,7 +24,7 @@ class Sequential { std::size_t next_; }; -// Simulates an uniformely distributed access pattern to a range of +// Simulates an uniformly distributed access pattern to a range of // [0,...,size). class Uniform { public: diff --git a/cpp/backend/common/eviction_policy.cc b/cpp/backend/common/eviction_policy.cc index 17450d1d7..429591de6 100644 --- a/cpp/backend/common/eviction_policy.cc +++ b/cpp/backend/common/eviction_policy.cc @@ -11,7 +11,7 @@ namespace carmen::backend { namespace { -// Selects a alement for the given set to be evicted according to the provided +// Selects an element for the given set to be evicted according to the provided // eviction pattern. The EvictionPattern needs to provide a Next() function // providing a random slot to be evicted. The next higher value present in the // set is then chosen to be evicted. @@ -106,7 +106,7 @@ void LeastRecentlyUsedEvictionPolicy::Read(std::size_t position) { } void LeastRecentlyUsedEvictionPolicy::Written(std::size_t position) { - // This policy does not distinguish between read an writes. + // This policy does not distinguish between read and writes. Read(position); } diff --git a/cpp/backend/common/eviction_policy_benchmark.cc b/cpp/backend/common/eviction_policy_benchmark.cc index 190437d5a..19ff42039 100644 --- a/cpp/backend/common/eviction_policy_benchmark.cc +++ b/cpp/backend/common/eviction_policy_benchmark.cc @@ -59,7 +59,7 @@ BENCHMARK(BM_UniformWriteTest) BENCHMARK(BM_UniformWriteTest) ->Range(kMinPoolSize, kMaxPoolSize); -// Evalutes the performance of removing elements from the pool. +// Evaluates the performance of removing elements from the pool. template void BM_UniformRemoveTest(benchmark::State& state) { auto pool_size = state.range(0); @@ -81,7 +81,7 @@ BENCHMARK(BM_UniformRemoveTest) BENCHMARK(BM_UniformRemoveTest) ->Range(kMinPoolSize, kMaxPoolSize); -// Evalutes the performance of selecting pages to be evicted. +// Evaluates the performance of selecting pages to be evicted. template void BM_GetPageToEvictTest(benchmark::State& state) { auto pool_size = state.range(0); diff --git a/cpp/backend/common/file.cc b/cpp/backend/common/file.cc index 2bc6bf2ad..5153fbbb0 100644 --- a/cpp/backend/common/file.cc +++ b/cpp/backend/common/file.cc @@ -4,18 +4,48 @@ #include #include +#include +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/strings/str_format.h" #include "backend/common/page.h" +#include "common/fstream.h" +#include "common/status_util.h" namespace carmen::backend { -// Creates the provided directory file path recursively. Returns true on -// success, false otherwise. -bool CreateDirectory(std::filesystem::path dir) { - if (std::filesystem::exists(dir)) return true; - if (!dir.has_relative_path()) return false; - return CreateDirectory(dir.parent_path()) && - std::filesystem::create_directory(dir); +// Creates the provided directory file path recursively. If the directory +// fails to be created, returns an error status. +absl::Status CreateDirectory(const std::filesystem::path& dir) { + if (std::filesystem::exists(dir)) return absl::OkStatus(); + if (!dir.has_relative_path()) { + return absl::InternalError( + absl::StrFormat("Failed to create directory %s.", dir)); + } + RETURN_IF_ERROR(CreateDirectory(dir.parent_path())); + if (!std::filesystem::create_directory(dir)) { + return absl::InternalError( + absl::StrFormat("Failed to create directory %s.", dir)); + } + return absl::OkStatus(); +} + +// Creates empty file at the provided file path. If the directory path +// does not exist, it is created. Returns ok status if the file was created +// successfully, otherwise returns the error status. +absl::Status CreateFile(const std::filesystem::path& path) { + if (std::filesystem::exists(path)) { + return absl::OkStatus(); + } + // Create the directory path if it does not exist. + RETURN_IF_ERROR(CreateDirectory(path.parent_path())); + // Opening the file write-only first creates the file in case it does not + // exist. + ASSIGN_OR_RETURN(auto fs, + FStream::Open(path, std::ios::binary | std::ios::out)); + RETURN_IF_ERROR(fs.Close()); + return absl::OkStatus(); } namespace internal { @@ -27,198 +57,335 @@ alignas(kFileSystemPageSize) static const std::array kZeros{}; } // namespace -FStreamFile::FStreamFile(std::filesystem::path file) { - // Create the parent directory. - CreateDirectory(file.parent_path()); - // Opening the file write-only first creates the file in case it does not - // exist. - data_.open(file, std::ios::binary | std::ios::out); - data_.close(); - // However, we need the file open in read & write mode. - data_.open(file, std::ios::binary | std::ios::out | std::ios::in); - data_.seekg(0, std::ios::end); - file_size_ = data_.tellg(); +absl::StatusOr FStreamFile::Open( + const std::filesystem::path& path) { + RETURN_IF_ERROR(CreateFile(path)); + ASSIGN_OR_RETURN( + auto fs, + FStream::Open(path, std::ios::binary | std::ios::in | std::ios::out)); + RETURN_IF_ERROR(fs.Seekg(0, std::ios::end)); + ASSIGN_OR_RETURN(auto file_size, fs.Tellg()); + return FStreamFile(std::move(fs), file_size); } -FStreamFile::~FStreamFile() { Close(); } +FStreamFile::FStreamFile(FStream fs, std::size_t file_size) + : file_size_(file_size), data_(std::move(fs)) {} + +FStreamFile::~FStreamFile() { Close().IgnoreError(); } -std::size_t FStreamFile::GetFileSize() { return file_size_; } +std::size_t FStreamFile::GetFileSize() const { return file_size_; } -void FStreamFile::Read(std::size_t pos, std::span span) { +absl::Status FStreamFile::Read(std::size_t pos, std::span span) { if (pos + span.size() > file_size_) { assert(pos >= file_size_ && "Reading non-aligned pages!"); - memset(span.data(), 0, span.size()); - return; + std::memset(span.data(), 0, span.size()); + return absl::OkStatus(); } - data_.seekg(pos); - data_.read(reinterpret_cast(span.data()), span.size()); + RETURN_IF_ERROR(data_.Seekg(pos)); + return data_.Read(span); } -void FStreamFile::Write(std::size_t pos, std::span span) { +absl::Status FStreamFile::Write(std::size_t pos, + std::span span) { // Grow file as needed. - GrowFileIfNeeded(pos + span.size()); - data_.seekp(pos); - data_.write(reinterpret_cast(span.data()), span.size()); + RETURN_IF_ERROR(GrowFileIfNeeded(pos + span.size())); + RETURN_IF_ERROR(data_.Seekp(pos)); + return data_.Write(span); } -void FStreamFile::Flush() { std::flush(data_); } +absl::Status FStreamFile::Flush() { return data_.Flush(); } -void FStreamFile::Close() { - Flush(); - data_.close(); +absl::Status FStreamFile::Close() { + if (data_.IsOpen()) { + RETURN_IF_ERROR(Flush()); + return data_.Close(); + } + return absl::OkStatus(); } -void FStreamFile::GrowFileIfNeeded(std::size_t needed) { +absl::Status FStreamFile::GrowFileIfNeeded(std::size_t needed) { if (file_size_ >= needed) { - return; + return absl::OkStatus(); } - data_.seekp(0, std::ios::end); + RETURN_IF_ERROR(data_.Seekp(0, std::ios::end)); while (file_size_ < needed) { auto step = std::min(kZeros.size(), needed - file_size_); - data_.write(kZeros.data(), step); + RETURN_IF_ERROR(data_.Write(std::span(kZeros.data(), step))); file_size_ += step; } + return absl::OkStatus(); } -CFile::CFile(std::filesystem::path file) { - // Create the parent directory. - CreateDirectory(file.parent_path()); - // Append mode will create the file if does not exist. - file_ = std::fopen(file.string().c_str(), "a"); - std::fclose(file_); - // But for read/write we need the file to be openend in expended read mode. - file_ = std::fopen(file.string().c_str(), "r+b"); - assert(file_); - [[maybe_unused]] auto succ = std::fseek(file_, 0, SEEK_END); - assert(succ == 0); - file_size_ = std::ftell(file_); +absl::StatusOr CFile::Open(const std::filesystem::path& path) { + // Create the file if it does not exist. + RETURN_IF_ERROR(CreateFile(path)); + // Open the file + auto file = std::fopen(path.string().c_str(), "r+b"); + if (file == nullptr) { + return absl::InternalError( + absl::StrFormat("Failed to open file %s.", path)); + } + // Seek to the end to get the file. + if (std::fseek(file, 0, SEEK_END) != 0) { + return absl::InternalError( + absl::StrFormat("Failed to seek to end of file %s.", path)); + } + // Get the file size. + auto file_size = std::ftell(file); + if (file_size == -1) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to get size of file %s.", path.string())); + } + return CFile(file, file_size); +} + +CFile::CFile(std::FILE* file, std::size_t file_size) + : file_size_(file_size), file_(file) {} + +CFile::CFile(CFile&& file) noexcept + : file_size_(file.file_size_), file_(file.file_) { + file.file_ = nullptr; } -CFile::~CFile() { Close(); } +CFile::~CFile() { Close().IgnoreError(); } -std::size_t CFile::GetFileSize() { return file_size_; } +std::size_t CFile::GetFileSize() const { return file_size_; } -void CFile::Read(std::size_t pos, std::span span) { - if (file_ == nullptr) return; +absl::Status CFile::Read(std::size_t pos, std::span span) { + if (file_ == nullptr) { + return absl::InternalError("File is not open."); + } if (pos + span.size() > file_size_) { assert(pos >= file_size_ && "Reading non-aligned pages!"); - memset(span.data(), 0, span.size()); - return; + std::memset(span.data(), 0, span.size()); + return absl::OkStatus(); + } + if (std::fseek(file_, pos, SEEK_SET) != 0) { + return absl::InternalError( + absl::StrFormat("Failed to seek to position %d.", pos)); + } + auto len = std::fread(span.data(), sizeof(std::byte), span.size(), file_); + if (len != span.size()) { + if (std::feof(file_)) { + return absl::InternalError(absl::StrFormat( + "Failed to read %d bytes from file. End of file reached.", + span.size())); + } + if (std::ferror(file_)) { + return absl::InternalError( + absl::StrFormat("Failed to read %d bytes from file.", span.size())); + } + return absl::InternalError( + absl::StrFormat("Read different number of bytes than requested." + " Requested: %d, Read: %d.", + span.size(), len)); } - [[maybe_unused]] auto succ = std::fseek(file_, pos, SEEK_SET); - assert(succ == 0); - [[maybe_unused]] auto len = - std::fread(span.data(), sizeof(std::byte), span.size(), file_); - assert(len == span.size()); + return absl::OkStatus(); } -void CFile::Write(std::size_t pos, std::span span) { - if (file_ == nullptr) return; +absl::Status CFile::Write(std::size_t pos, std::span span) { + if (file_ == nullptr) { + return absl::InternalError("File is not open."); + } // Grow file as needed. - GrowFileIfNeeded(pos + span.size()); - [[maybe_unused]] auto succ = std::fseek(file_, pos, SEEK_SET); - assert(succ == 0); - [[maybe_unused]] auto len = - std::fwrite(span.data(), sizeof(std::byte), span.size(), file_); - assert(len == span.size()); + RETURN_IF_ERROR(GrowFileIfNeeded(pos + span.size())); + if (std::fseek(file_, pos, SEEK_SET) != 0) { + return absl::InternalError( + absl::StrFormat("Failed to seek to position %d.", pos)); + } + auto len = std::fwrite(span.data(), sizeof(std::byte), span.size(), file_); + if (len != span.size()) { + if (std::ferror(file_)) { + return absl::InternalError( + absl::StrFormat("Failed to write %d bytes to file.", span.size())); + } + return absl::InternalError( + absl::StrFormat("Wrote different number of bytes than requested." + " Requested: %d, Written: %d.", + span.size(), len)); + } + return absl::OkStatus(); } -void CFile::Flush() { - if (file_ == nullptr) return; - std::fflush(file_); +absl::Status CFile::Flush() { + if (file_ != nullptr && std::fflush(file_) == EOF) { + return absl::InternalError("Failed to flush file."); + } + return absl::OkStatus(); } -void CFile::Close() { - if (file_ == nullptr) return; - Flush(); - fclose(file_); - file_ = nullptr; +absl::Status CFile::Close() { + if (file_ != nullptr) { + RETURN_IF_ERROR(Flush()); + if (std::fclose(file_) == EOF) { + return absl::InternalError("Failed to close file."); + } + file_ = nullptr; + } + return absl::OkStatus(); } -void CFile::GrowFileIfNeeded(std::size_t needed) { +absl::Status CFile::GrowFileIfNeeded(std::size_t needed) { if (file_size_ >= needed) { - return; + return absl::OkStatus(); + } + if (std::fseek(file_, 0, SEEK_END) != 0) { + return absl::InternalError( + absl::StrFormat("Failed to seek to end of file.")); } - std::fseek(file_, 0, SEEK_END); while (file_size_ < needed) { auto step = std::min(kZeros.size(), needed - file_size_); - [[maybe_unused]] auto len = - fwrite(kZeros.data(), sizeof(std::byte), step, file_); - assert(len == step); + auto len = std::fwrite(kZeros.data(), sizeof(std::byte), step, file_); + if (len != step) { + if (std::ferror(file_)) { + return absl::InternalError( + absl::StrFormat("Failed to write %d bytes to file.", step)); + } + return absl::InternalError( + absl::StrFormat("Wrote different number of bytes than requested." + " Requested: %d, Written: %d.", + step, len)); + } file_size_ += step; } + return absl::OkStatus(); } -PosixFile::PosixFile(std::filesystem::path file) { +absl::StatusOr PosixFile::Open(const std::filesystem::path& path) { // Create the parent directory. - CreateDirectory(file.parent_path()); + RETURN_IF_ERROR(CreateDirectory(path.parent_path())); + int fd; #ifdef O_DIRECT // When using O_DIRECT, all read/writes must use aligned memory locations! - fd_ = open(file.string().c_str(), O_CREAT | O_DIRECT | O_RDWR); + fd = open(path.string().c_str(), O_CREAT | O_DIRECT | O_RDWR); #else - fd_ = open(file.string().c_str(), O_CREAT | O_RDWR); + fd = open(path.string().c_str(), O_CREAT | O_RDWR); #endif - assert(fd_ >= 0); - off_t size = lseek(fd_, 0, SEEK_END); + // Open the file. + if (fd == -1) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to open file %s.", path.string())); + } + // Seek to the end to get the file. + off_t size = lseek(fd, 0, SEEK_END); if (size == -1) { - perror("Error getting file size: "); + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to seek to end of file %s.", path.string())); } - file_size_ = size; + return PosixFile(fd, size); +} + +PosixFile::PosixFile(int fd, std::size_t file_size) + : file_size_(file_size), fd_(fd) {} + +PosixFile::PosixFile(PosixFile&& file) noexcept + : file_size_(file.file_size_), fd_(file.fd_) { + file.fd_ = -1; } -PosixFile::~PosixFile() { Close(); } +PosixFile::~PosixFile() { Close().IgnoreError(); } -std::size_t PosixFile::GetFileSize() { return file_size_; } +std::size_t PosixFile::GetFileSize() const { return file_size_; } -void PosixFile::Read(std::size_t pos, std::span span) { - if (fd_ < 0) return; +absl::Status PosixFile::Read(std::size_t pos, std::span span) { + if (fd_ < 0) { + return absl::InternalError("File is not open."); + } if (pos + span.size() > file_size_) { assert(pos >= file_size_ && "Reading non-aligned pages!"); - memset(span.data(), 0, span.size()); - return; + std::memset(span.data(), 0, span.size()); + return absl::OkStatus(); + } + RETURN_IF_ERROR(GrowFileIfNeeded(pos + span.size())); + if (lseek(fd_, pos, SEEK_SET) == -1) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to seek to position %d.", pos)); } - GrowFileIfNeeded(pos + span.size()); - lseek(fd_, pos, SEEK_SET); - [[maybe_unused]] auto len = read(fd_, span.data(), span.size()); - assert(len == static_cast(span.size())); + auto len = read(fd_, span.data(), span.size()); + if (len != static_cast(span.size())) { + if (len == -1) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to read %d bytes from file.", span.size())); + } + return absl::InternalError( + absl::StrFormat("Failed to read %d bytes from file.", span.size())); + } + return absl::OkStatus(); } -void PosixFile::Write(std::size_t pos, std::span span) { - if (fd_ < 0) return; +absl::Status PosixFile::Write(std::size_t pos, + std::span span) { + if (fd_ < 0) { + return absl::InternalError("File is not open."); + } // Grow file as needed. - GrowFileIfNeeded(pos + span.size()); - lseek(fd_, pos, SEEK_SET); - [[maybe_unused]] auto len = write(fd_, span.data(), span.size()); - assert(len == static_cast(span.size())); + RETURN_IF_ERROR(GrowFileIfNeeded(pos + span.size())); + if (lseek(fd_, pos, SEEK_SET) == -1) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Failed to seek to position %d.", pos)); + } + auto len = write(fd_, span.data(), span.size()); + if (len != static_cast(span.size())) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Wrote different number of bytes than requested." + "Wrote %d, requested %d.", + len, span.size())); + } + return absl::OkStatus(); } -void PosixFile::Flush() { - if (fd_ < 0) return; - fsync(fd_); +absl::Status PosixFile::Flush() { + if (fd_ >= 0 && fsync(fd_) == -1) { + return GetStatusWithSystemError(absl::StatusCode::kInternal, errno, + "Failed to flush file."); + } + return absl::OkStatus(); } -void PosixFile::Close() { - if (fd_ < 0) return; - Flush(); - close(fd_); - fd_ = -1; +absl::Status PosixFile::Close() { + if (fd_ >= 0) { + RETURN_IF_ERROR(Flush()); + if (close(fd_) == -1) { + return GetStatusWithSystemError(absl::StatusCode::kInternal, errno, + "Failed to close file."); + } + fd_ = -1; + } + return absl::OkStatus(); } -void PosixFile::GrowFileIfNeeded(std::size_t needed) { +absl::Status PosixFile::GrowFileIfNeeded(std::size_t needed) { if (file_size_ >= needed) { - return; + return absl::OkStatus(); + } + auto offset = lseek(fd_, 0, SEEK_END); + if (offset != static_cast(file_size_)) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat( + "Failed to seek to end of file. Expected offset %d, got %d.", + file_size_, offset)); } - [[maybe_unused]] auto offset = lseek(fd_, 0, SEEK_END); - assert(offset == static_cast(file_size_)); while (file_size_ < needed) { auto step = std::min(kZeros.size(), needed - file_size_); auto len = write(fd_, kZeros.data(), step); - if (len < 0) { - perror("Error growing file"); + if (len != static_cast(step)) { + return GetStatusWithSystemError( + absl::StatusCode::kInternal, errno, + absl::StrFormat("Wrote different number of bytes than requested." + "Expected: %d, actual: %d", + step, len)); } - assert(len == static_cast(step)); file_size_ += step; } + return absl::OkStatus(); } } // namespace internal diff --git a/cpp/backend/common/file.h b/cpp/backend/common/file.h index 79eeb47f7..24a292d0e 100644 --- a/cpp/backend/common/file.h +++ b/cpp/backend/common/file.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -8,7 +9,12 @@ #include #include +#include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "backend/common/page.h" #include "backend/common/page_id.h" +#include "common/fstream.h" +#include "common/status_util.h" namespace carmen::backend { @@ -16,7 +22,11 @@ namespace carmen::backend { // Creates the provided directory file path recursively in case it does not // exit. Returns true if the directory exists after the call, false otherwise. -bool CreateDirectory(std::filesystem::path dir); +absl::Status CreateDirectory(const std::filesystem::path& dir); + +// Creates empty file at the provided file path. Returns ok status if the file +// was created successfully, otherwise returns the error status. +absl::Status CreateFile(const std::filesystem::path& path); // The File concept defines an interface for file implementations supporting the // loading and storing of fixed length pages. Pages are expected to be numbered @@ -27,24 +37,29 @@ concept File = requires(F a) { std::is_move_constructible_v; std::is_move_assignable_v; + // All files must be open-able through a static factory function. + { + F::Open(std::declval()) + } -> std::same_as>; + // Each file implementation must support the extraction of the number of // pages. { a.GetNumPages() } -> std::same_as; // LoadPage is intended to be used for fetching a single page from the file. { a.LoadPage(PageId{}, std::declval>()) - } -> std::same_as; + } -> std::same_as; // StorePage is intended to be used for fetching a single page from the file. { a.StorePage(PageId{}, std::declval>()) - } -> std::same_as; + } -> std::same_as; // Each file has to support a flush operation after which data previously // written must be persisted on disk. - { a.Flush() } -> std::same_as; + { a.Flush() } -> std::same_as; // Each file has to support a close operation, flushing buffered data and // releasing file resources. After a file is closed it may no longer be used. - { a.Close() } -> std::same_as; + { a.Close() } -> std::same_as; }; // An InMemoryFile implement is provided to for testing purposes, where actual @@ -55,21 +70,26 @@ class InMemoryFile { public: constexpr static std::size_t kPageSize = page_size; + static absl::StatusOr Open(const std::filesystem::path&) { + return InMemoryFile(); + } + InMemoryFile() = default; - InMemoryFile(std::filesystem::path){}; std::size_t GetNumPages() const { return data_.size(); } - void LoadPage(PageId id, std::span trg) const; + absl::Status LoadPage(PageId id, std::span trg) const; - void StorePage(PageId id, std::span src); + absl::Status StorePage(PageId id, std::span src); - void Flush() const { + absl::Status Flush() const { // Nothing to do. + return absl::OkStatus(); } - void Close() const { + absl::Status Close() const { // Nothing to do. + return absl::OkStatus(); } private: @@ -84,70 +104,82 @@ namespace internal { // implementations. Note: FStreamFile is not satisfying any File concept. class FStreamFile { public: - // Opens the given file in read/write mode. If it does not exist, the file is - // created. TODO(herbertjordan): add error handling. - FStreamFile(std::filesystem::path file); + // Opens the file at the provided path. If the file does not exist it will be + // created. + static absl::StatusOr Open(const std::filesystem::path& path); + + // Assure the file is move constructable. + FStreamFile(FStreamFile&&) noexcept = default; + // Flushes the content and closes the file. ~FStreamFile(); // Provides the current file size in bytes. - std::size_t GetFileSize(); + std::size_t GetFileSize() const; // Reads a range of bytes from the file to the given span. The provided // position is the starting position. The number of bytes to be read is taken // from the length of the provided span. - void Read(std::size_t pos, std::span span); + absl::Status Read(std::size_t pos, std::span span); // Writes a span of bytes to the file at the given position. If needed, the // file is grown to fit all the data of the span. Additional bytes between the // current end and the starting position are initialized with zeros. - void Write(std::size_t pos, std::span span); + absl::Status Write(std::size_t pos, std::span span); // Flushes all pending/buffered writes to disk. - void Flush(); + absl::Status Flush(); // Flushes the file and closes the underlying resource. - void Close(); + absl::Status Close(); private: + FStreamFile(FStream fs, std::size_t file_size); + // Grows the underlying file to the given size. - void GrowFileIfNeeded(std::size_t needed); + absl::Status GrowFileIfNeeded(std::size_t needed); std::size_t file_size_; - std::fstream data_; + FStream data_; }; // A CFile provides raw read/write access to a file C's stdio.h header. class CFile { public: // Opens the given file in read/write mode. If it does not exist, the file is - // created. TODO(herbertjordan): add error handling. - CFile(std::filesystem::path file); + // created. + static absl::StatusOr Open(const std::filesystem::path& path); + + // Assure the file is move constructable. + CFile(CFile&&) noexcept; + // Flushes the content and closes the file. ~CFile(); // Provides the current file size in bytes. - std::size_t GetFileSize(); + std::size_t GetFileSize() const; // Reads a range of bytes from the file to the given span. The provided // position is the starting position. The number of bytes to be read is taken // from the length of the provided span. - void Read(std::size_t pos, std::span span); + absl::Status Read(std::size_t pos, std::span span); // Writes a span of bytes to the file at the given position. If needed, the // file is grown to fit all the data of the span. Additional bytes between the // current end and the starting position are initialized with zeros. - void Write(std::size_t pos, std::span span); + absl::Status Write(std::size_t pos, std::span span); // Flushes all pending/buffered writes to disk. - void Flush(); + absl::Status Flush(); // Flushes the file and closes the underlying resource. - void Close(); + absl::Status Close(); private: + CFile(std::FILE* file, std::size_t file_size); + // Grows the underlying file to the given size. - void GrowFileIfNeeded(std::size_t needed); + absl::Status GrowFileIfNeeded(std::size_t needed); std::size_t file_size_; std::FILE* file_; @@ -157,33 +189,39 @@ class CFile { class PosixFile { public: // Opens the given file in read/write mode. If it does not exist, the file is - // created. TODO(herbertjordan): add error handling. - PosixFile(std::filesystem::path file); + // created. + static absl::StatusOr Open(const std::filesystem::path& path); + + // Assure the file is move constructable. + PosixFile(PosixFile&&) noexcept; + // Flushes the content and closes the file. ~PosixFile(); // Provides the current file size in bytes. - std::size_t GetFileSize(); + std::size_t GetFileSize() const; // Reads a range of bytes from the file to the given span. The provided // position is the starting position. The number of bytes to be read is taken // from the length of the provided span. - void Read(std::size_t pos, std::span span); + absl::Status Read(std::size_t pos, std::span span); // Writes a span of bytes to the file at the given position. If needed, the // file is grown to fit all the data of the span. Additional bytes between the // current end and the starting position are initialized with zeros. - void Write(std::size_t pos, std::span span); + absl::Status Write(std::size_t pos, std::span span); // Flushes all pending/buffered writes to disk. - void Flush(); + absl::Status Flush(); // Flushes the file and closes the underlying resource. - void Close(); + absl::Status Close(); private: + PosixFile(int fd, std::size_t file_size); + // Grows the underlying file to the given size. - void GrowFileIfNeeded(std::size_t needed); + absl::Status GrowFileIfNeeded(std::size_t needed); std::size_t file_size_; int fd_; @@ -198,23 +236,29 @@ class SingleFileBase { public: constexpr static std::size_t kPageSize = page_size; - SingleFileBase(std::filesystem::path file_path) : file_(file_path) {} + static absl::StatusOr Open( + const std::filesystem::path& path) { + ASSIGN_OR_RETURN(auto file, RawFile::Open(path)); + return SingleFileBase(std::move(file)); + } std::size_t GetNumPages() const { return file_.GetFileSize() / page_size; } - void LoadPage(PageId id, std::span trg) const { - file_.Read(id * page_size, trg); + absl::Status LoadPage(PageId id, std::span trg) const { + return file_.Read(id * page_size, trg); } - void StorePage(PageId id, std::span src) { - file_.Write(id * page_size, src); + absl::Status StorePage(PageId id, std::span src) { + return file_.Write(id * page_size, src); } - void Flush() { file_.Flush(); } + absl::Status Flush() { return file_.Flush(); } - void Close() { file_.Close(); } + absl::Status Close() { return file_.Close(); } private: + SingleFileBase(RawFile file) : file_(std::move(file)) {} + mutable RawFile file_; }; @@ -227,20 +271,22 @@ using SingleFile = SingleFileBase; // ------------------------------- Definitions -------------------------------- template -void InMemoryFile::LoadPage( +absl::Status InMemoryFile::LoadPage( PageId id, std::span trg) const { static const Block zero{}; auto src = id >= data_.size() ? &zero : &data_[id]; std::memcpy(trg.data(), src, page_size); + return absl::OkStatus(); } template -void InMemoryFile::StorePage( +absl::Status InMemoryFile::StorePage( PageId id, std::span src) { while (data_.size() <= id) { data_.resize(id + 1); } std::memcpy(&data_[id], src.data(), page_size); + return absl::OkStatus(); } } // namespace carmen::backend diff --git a/cpp/backend/common/file_benchmark.cc b/cpp/backend/common/file_benchmark.cc index 5f43c4296..6a6f81fad 100644 --- a/cpp/backend/common/file_benchmark.cc +++ b/cpp/backend/common/file_benchmark.cc @@ -2,10 +2,12 @@ #include #include +#include "absl/status/statusor.h" #include "backend/common/file.h" #include "backend/common/page.h" #include "benchmark/benchmark.h" #include "common/file_util.h" +#include "common/status_test_util.h" namespace carmen::backend::store { namespace { @@ -28,8 +30,8 @@ namespace { template using Page = RawPage; -// An utility wrapper to be specialized for various file implementations to -// handle them uniformely within benchmarks. +// A utility wrapper to be specialized for various file implementations to +// handle them uniformly within benchmarks. // // The default implementation maintains a File instance and the ownership of a // temporary file on disk backing the owned file instance. In particular, it @@ -38,30 +40,32 @@ using Page = RawPage; template class FileWrapper { public: - FileWrapper() : file_(std::make_unique(temp_file_)) {} + static absl::StatusOr Create() { + TempFile temp_file; + ASSIGN_OR_RETURN(auto file, F::Open(temp_file.GetPath())); + return FileWrapper(std::make_unique(std::move(file)), + std::move(temp_file)); + } + + FileWrapper(FileWrapper&&) noexcept = default; + ~FileWrapper() { - file_->Flush(); - file_.reset(); - std::filesystem::remove(temp_file_); + if (file_) { + file_->Flush().IgnoreError(); + file_.reset(); + } } + F& GetFile() { return *file_; } private: + FileWrapper(std::unique_ptr file, TempFile temp_file) + : temp_file_(std::move(temp_file)), file_(std::move(file)) {} + TempFile temp_file_; std::unique_ptr file_; }; -// A specialization of a FileWrapper for the InMemoryFile reference -// implementation. -template -class FileWrapper> { - public: - InMemoryFile& GetFile() { return file_; } - - private: - InMemoryFile file_; -}; - template using StreamFile = SingleFileBase; @@ -83,10 +87,10 @@ void BM_FileInit(benchmark::State& state) { for (auto _ : state) { // We create a file and only write the final page. This implicitly creates // the rest of the file. - FileWrapper wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, FileWrapper::Create()); F& file = wrapper.GetFile(); Page trg; - file.StorePage(target_size / sizeof(trg) - 1, trg); + ASSERT_OK(file.StorePage(target_size / sizeof(trg) - 1, trg)); benchmark::DoNotOptimize(trg[0]); } } @@ -118,11 +122,11 @@ void BM_SequentialFileFilling(benchmark::State& state) { const auto target_size = state.range(0); for (auto _ : state) { - FileWrapper wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, FileWrapper::Create()); F& file = wrapper.GetFile(); for (std::size_t i = 0; i < target_size / F::kPageSize; i++) { Page trg; - file.StorePage(i, trg); + ASSERT_OK(file.StorePage(i, trg)); benchmark::DoNotOptimize(trg[0]); } } @@ -162,16 +166,16 @@ void BM_SequentialFileRead(benchmark::State& state) { const auto target_size = state.range(0); // Create and initialize the test file. - FileWrapper wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, FileWrapper::Create()); F& file = wrapper.GetFile(); Page trg; const auto num_pages = target_size / F::kPageSize; - file.StorePage(num_pages - 1, trg); + ASSERT_OK(file.StorePage(num_pages - 1, trg)); int i = 0; for (auto _ : state) { // Load all pages in order. - file.LoadPage(i++ % num_pages, trg); + ASSERT_OK(file.LoadPage(i++ % num_pages, trg)); benchmark::DoNotOptimize(trg[0]); } } @@ -203,11 +207,11 @@ void BM_RandomFileRead(benchmark::State& state) { const auto target_size = state.range(0); // Create and initialize the test file. - FileWrapper wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, FileWrapper::Create()); F& file = wrapper.GetFile(); Page trg; const auto num_pages = target_size / F::kPageSize; - file.StorePage(num_pages - 1, trg); + ASSERT_OK(file.StorePage(num_pages - 1, trg)); std::random_device rd; std::mt19937 gen(rd()); @@ -215,7 +219,7 @@ void BM_RandomFileRead(benchmark::State& state) { for (auto _ : state) { // Load pages in random order. - file.LoadPage(distribution(gen), trg); + ASSERT_OK(file.LoadPage(distribution(gen), trg)); benchmark::DoNotOptimize(trg[0]); } } diff --git a/cpp/backend/common/file_test.cc b/cpp/backend/common/file_test.cc index f34d84d0b..d3cd5ce69 100644 --- a/cpp/backend/common/file_test.cc +++ b/cpp/backend/common/file_test.cc @@ -5,6 +5,7 @@ #include "backend/common/page.h" #include "common/file_util.h" +#include "common/status_test_util.h" #include "gtest/gtest.h" namespace carmen::backend::store { @@ -39,67 +40,67 @@ TEST(InMemoryFileTest, IsFile) { } TEST(InMemoryFileTest, InitialFileIsEmpty) { - InMemoryFile<32> file; + ASSERT_OK_AND_ASSIGN(auto file, InMemoryFile<32>::Open("")); EXPECT_EQ(0, file.GetNumPages()); } TEST(InMemoryFileTest, PagesCanBeWrittenAndRead) { using Page = Page<8>; - InMemoryFile file; + ASSERT_OK_AND_ASSIGN(auto file, InMemoryFile::Open("")); Page page_a{std::byte{0x01}, std::byte{0x02}}; - file.StorePage(0, page_a); + ASSERT_OK(file.StorePage(0, page_a)); EXPECT_EQ(1, file.GetNumPages()); Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(page_a, restored); } TEST(InMemoryFileTest, PagesAreDifferentiated) { using Page = Page<4>; - InMemoryFile file; + ASSERT_OK_AND_ASSIGN(auto file, InMemoryFile::Open("")); Page page_a{std::byte{0x01}, std::byte{0x02}}; Page page_b{std::byte{0x03}, std::byte{0x04}}; - file.StorePage(0, page_a); - file.StorePage(1, page_b); + ASSERT_OK(file.StorePage(0, page_a)); + ASSERT_OK(file.StorePage(1, page_b)); EXPECT_EQ(2, file.GetNumPages()); Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(page_a, restored); - file.LoadPage(1, restored); + ASSERT_OK(file.LoadPage(1, restored)); EXPECT_EQ(page_b, restored); } TEST(InMemoryFileTest, WritingPagesCreatesImplicitEmptyPages) { using Page = Page<8>; - InMemoryFile file; + ASSERT_OK_AND_ASSIGN(auto file, InMemoryFile::Open("")); // Storing a page at position 2 implicitly creates pages 0 and 1. Page page_a{std::byte{0x01}, std::byte{0x02}}; - file.StorePage(2, page_a); + ASSERT_OK(file.StorePage(2, page_a)); EXPECT_EQ(3, file.GetNumPages()); Page zero{}; Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(zero, restored); - file.LoadPage(1, restored); + ASSERT_OK(file.LoadPage(1, restored)); EXPECT_EQ(zero, restored); - file.LoadPage(2, restored); + ASSERT_OK(file.LoadPage(2, restored)); EXPECT_EQ(page_a, restored); } TEST(InMemoryFileTest, LoadingUninitializedPagesLeadsToZeros) { using Page = Page<4>; - InMemoryFile file; + ASSERT_OK_AND_ASSIGN(auto file, InMemoryFile::Open("")); Page zero{}; Page loaded; loaded.fill(std::byte{1}); - file.LoadPage(0, loaded); + ASSERT_OK(file.LoadPage(0, loaded)); EXPECT_EQ(zero, loaded); } @@ -115,26 +116,29 @@ TYPED_TEST_P(SingleFileTest, IsFile) { } TYPED_TEST_P(SingleFileTest, ExistingFileCanBeOpened) { + using File = SingleFileBase<32, TypeParam>; TempFile temp_file; ASSERT_TRUE(std::filesystem::exists(temp_file.GetPath())); - SingleFileBase<32, TypeParam> file(temp_file.GetPath()); + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); EXPECT_EQ(0, file.GetNumPages()); } TYPED_TEST_P(SingleFileTest, NonExistingFileIsCreated) { + using File = SingleFileBase<32, TypeParam>; TempFile temp_file; ASSERT_TRUE(std::filesystem::exists(temp_file.GetPath())); std::filesystem::remove(temp_file.GetPath()); ASSERT_FALSE(std::filesystem::exists(temp_file.GetPath())); - SingleFileBase<32, TypeParam> file(temp_file.GetPath()); + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); EXPECT_TRUE(std::filesystem::exists(temp_file.GetPath())); EXPECT_EQ(0, file.GetNumPages()); } TYPED_TEST_P(SingleFileTest, NestedDirectoryIsCreatedIfNeeded) { + using File = SingleFileBase<32, TypeParam>; TempDir temp_dir; - SingleFileBase<32, TypeParam> file(temp_dir.GetPath() / "some" / "dir" / - "file.dat"); + ASSERT_OK_AND_ASSIGN( + auto file, File::Open(temp_dir.GetPath() / "some" / "dir" / "file.dat")); EXPECT_TRUE(std::filesystem::exists(temp_dir.GetPath())); EXPECT_TRUE(std::filesystem::exists(temp_dir.GetPath() / "some")); EXPECT_TRUE(std::filesystem::exists(temp_dir.GetPath() / "some" / "dir")); @@ -144,72 +148,77 @@ TYPED_TEST_P(SingleFileTest, NestedDirectoryIsCreatedIfNeeded) { } TYPED_TEST_P(SingleFileTest, InitialFileIsEmpty) { + using File = SingleFileBase<32, TypeParam>; TempFile temp_file; - SingleFileBase<32, TypeParam> file(temp_file.GetPath()); + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); EXPECT_EQ(0, file.GetNumPages()); } TYPED_TEST_P(SingleFileTest, PagesCanBeWrittenAndRead) { using Page = Page; + using File = SingleFileBase; TempFile temp_file; - SingleFileBase file(temp_file.GetPath()); + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); Page page_a{std::byte{0x01}, std::byte{0x02}}; - file.StorePage(0, page_a); + ASSERT_OK(file.StorePage(0, page_a)); EXPECT_EQ(1, file.GetNumPages()); Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(page_a, restored); } TYPED_TEST_P(SingleFileTest, PagesAreDifferentiated) { - TempFile temp_file; using Page = Page; - SingleFileBase file(temp_file.GetPath()); + using File = SingleFileBase; + TempFile temp_file; + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); Page page_a{std::byte{0x01}, std::byte{0x02}}; Page page_b{std::byte{0x03}, std::byte{0x04}}; - file.StorePage(0, page_a); - file.StorePage(1, page_b); + ASSERT_OK(file.StorePage(0, page_a)); + ASSERT_OK(file.StorePage(1, page_b)); EXPECT_EQ(2, file.GetNumPages()); Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(page_a, restored); - file.LoadPage(1, restored); + ASSERT_OK(file.LoadPage(1, restored)); EXPECT_EQ(page_b, restored); } TYPED_TEST_P(SingleFileTest, WritingPagesCreatesImplicitEmptyPages) { - TempFile temp_file; using Page = Page; - SingleFileBase file(temp_file.GetPath()); + using File = SingleFileBase; + TempFile temp_file; + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); // Storing a page at position 2 implicitly creates pages 0 and 1. Page page_a{std::byte{0x01}, std::byte{0x02}}; - file.StorePage(2, page_a); + ASSERT_OK(file.StorePage(2, page_a)); EXPECT_EQ(3, file.GetNumPages()); Page zero{}; Page restored; - file.LoadPage(0, restored); + ASSERT_OK(file.LoadPage(0, restored)); EXPECT_EQ(zero, restored); - file.LoadPage(1, restored); + ASSERT_OK(file.LoadPage(1, restored)); EXPECT_EQ(zero, restored); - file.LoadPage(2, restored); + ASSERT_OK(file.LoadPage(2, restored)); EXPECT_EQ(page_a, restored); } TYPED_TEST_P(SingleFileTest, LoadingUninitializedPagesLeadsToZeros) { - TempFile temp_file; using Page = Page; - SingleFileBase file(temp_file.GetPath()); + using File = SingleFileBase; + TempFile temp_file; + ASSERT_OK_AND_ASSIGN(auto file, File::Open(temp_file.GetPath())); Page zero{}; Page loaded; loaded.fill(std::byte{1}); - file.LoadPage(0, loaded); + ASSERT_OK(file.LoadPage(0, loaded)); EXPECT_EQ(zero, loaded); } diff --git a/cpp/backend/common/leveldb/leveldb_test.cc b/cpp/backend/common/leveldb/leveldb_test.cc index ef6a817c9..9521a5f6a 100644 --- a/cpp/backend/common/leveldb/leveldb_test.cc +++ b/cpp/backend/common/leveldb/leveldb_test.cc @@ -9,6 +9,7 @@ namespace carmen::backend { namespace { using ::testing::IsOk; +using ::testing::IsOkAndHolds; using ::testing::Not; using ::testing::StrEq; @@ -27,25 +28,22 @@ TEST(LevelDb, TestAddAndGet) { TempDir dir; std::string key("key"); std::string value("value"); - auto db = *LevelDb::Open(dir.GetPath()); + ASSERT_OK_AND_ASSIGN(auto db, LevelDb::Open(dir.GetPath())); ASSERT_OK(db.Add({key, value})); - ASSERT_OK_AND_ASSIGN(auto result, db.Get(key)); - EXPECT_THAT(value, StrEq(result)); + EXPECT_THAT(db.Get(key), IsOkAndHolds(StrEq(value))); } TEST(LevelDb, TestAddBatchAndGet) { TempDir dir; - auto db = *LevelDb::Open(dir.GetPath()); + ASSERT_OK_AND_ASSIGN(auto db, LevelDb::Open(dir.GetPath())); std::string key1("key1"); std::string key2("key2"); std::string value1("value1"); std::string value2("value2"); auto input = std::array{LDBEntry{key1, value1}, LDBEntry{key2, value2}}; ASSERT_OK(db.AddBatch(input)); - ASSERT_OK_AND_ASSIGN(auto result1, db.Get(key1)); - ASSERT_OK_AND_ASSIGN(auto result2, db.Get(key2)); - EXPECT_THAT(value1, StrEq(result1)); - EXPECT_THAT(value2, StrEq(result2)); + EXPECT_THAT(db.Get(key1), IsOkAndHolds(StrEq(value1))); + EXPECT_THAT(db.Get(key2), IsOkAndHolds(StrEq(value2))); } } // namespace } // namespace carmen::backend diff --git a/cpp/backend/common/page.h b/cpp/backend/common/page.h index 001a0a372..d758e5ade 100644 --- a/cpp/backend/common/page.h +++ b/cpp/backend/common/page.h @@ -20,7 +20,7 @@ concept Page = alignof(P) >= kFileSystemPageSize && alignof(P) % kFileSystemPageSize == 0 && // To be used in page pools, pages must also be trivially default - // constructible and destructible. + // constructable and destructible. std::is_trivially_default_constructible_v

&& std:: is_trivially_destructible_v

&& std::is_convertible_v< P, std::span>&& std:: @@ -49,8 +49,8 @@ constexpr std::size_t GetRequiredPageSize(std::size_t needed_page_size) { template class alignas(kFileSystemPageSize) RawPage final { public: - // Can be used to interprate the content of this page using the given page - // format. It is a readability wrapper arround a static cast, performing no + // Can be used to interpret the content of this page using the given page + // format. It is a readability wrapper around a static cast, performing no // dynamic checks on the validity of the cast. template const Page& As() const { @@ -86,10 +86,10 @@ class alignas(kFileSystemPageSize) RawPage final { // typed version of a page in a file containing a fixed length array of trivial // elements. Furthermore, it provides index based access to the contained data. // -// The trival type V is the type of value stored in this page, in the form of an -// array. The provided num_elements is the number values per page. Note that, if -// total size of the array is not a multiple of kFileSystemPageSize some extra -// bytes per page may be kept in memory and on disk. +// The trivial type V is the type of value stored in this page, in the form of +// an array. The provided num_elements is the number values per page. Note that, +// if total size of the array is not a multiple of kFileSystemPageSize some +// extra bytes per page may be kept in memory and on disk. template class alignas(kFileSystemPageSize) ArrayPage final { public: diff --git a/cpp/backend/common/page_manager.h b/cpp/backend/common/page_manager.h index a30598da0..25a59a234 100644 --- a/cpp/backend/common/page_manager.h +++ b/cpp/backend/common/page_manager.h @@ -10,7 +10,7 @@ namespace carmen::backend { // A page manager is like a memory manager organizing the life cycle of pages in // a single file, accessed through a page pool. It allows to create (=allocate) -// new pages, resolve PageIDs to Pages (=dereferencign), and the freeing and +// new pages, resolve PageIDs to Pages (=dereferencing), and the freeing and // reusing of pages. // // NOTE: this is still work in progress; missing features: @@ -36,7 +36,8 @@ class PageManager { template absl::StatusOr> New() { PageId id = next_++; - return NewPage{id, pool_.template Get(id)}; + ASSIGN_OR_RETURN(Page & page, pool_.template Get(id)); + return NewPage{id, page}; } // Resolves a page ID to a page reference. It is the task of the caller to diff --git a/cpp/backend/common/page_pool.h b/cpp/backend/common/page_pool.h index 9ba815ae8..579a29076 100644 --- a/cpp/backend/common/page_pool.h +++ b/cpp/backend/common/page_pool.h @@ -5,10 +5,13 @@ #include #include "absl/container/flat_hash_map.h" +#include "absl/status/status.h" +#include "absl/status/statusor.h" #include "backend/common/eviction_policy.h" #include "backend/common/file.h" #include "backend/common/page.h" #include "common/memory_usage.h" +#include "common/status_util.h" namespace carmen::backend { @@ -23,7 +26,7 @@ class PagePoolListener; // // Each PagePool is backed by a file instance from which it fetches pages and to // which it writes modifications to. Furthermore, listeners may be registered, -// enabling the incjection of extra operations during page load and eviction +// enabling the injection of extra operations during page load and eviction // steps. template class PagePool { @@ -47,7 +50,7 @@ class PagePool { // the disk. This may require the eviction of another page. // Note: the returned reference is only valid until the next Get() call. template - Page& Get(PageId id); + StatusOrRef Get(PageId id); // Marks the given page as being modified. Thus, before it gets evicted from // the pool, it needs to be written back to the file. @@ -57,27 +60,27 @@ class PagePool { // Registers a page pool listener monitoring events. void AddListener(std::unique_ptr listener); - // Syncronizes all pages in the pool by writing all dirty pages out to disk. + // Synchronizes all pages in the pool by writing all dirty pages out to disk. // Does not affect the content of the pool, nor are page accesses reported to // the pool policies. - void Flush(); + absl::Status Flush(); // Flushes the content of this pool and released the underlying file. - void Close(); + absl::Status Close(); // Summarizes the memory usage of this instance. MemoryFootprint GetMemoryFootprint() const; - // Returns the access to the utilized eviction policicy, mainly for testing. + // Returns the access to the utilized eviction policy, mainly for testing. EvictionPolicy& GetEvictionPolicy() { return eviction_policy_; } private: // Obtains a free slot in the pool. If all are occupied, a page is evicted to // make space. - std::size_t GetFreeSlot(); + absl::StatusOr GetFreeSlot(); // Performs the eviction of a page at the given position. - void EvictSlot(int position); + absl::Status EvictSlot(int position); // The file used for loading and storing pages. std::unique_ptr file_; @@ -98,7 +101,7 @@ class PagePool { // stored to disk before being evicted. std::vector dirty_; - // Indexes recording which page is in which pool postion. + // Indexes recording which page is in which pool position. absl::flat_hash_map pages_to_index_; std::vector index_to_pages_; @@ -146,7 +149,7 @@ PagePool::PagePool(std::unique_ptr file, std::size_t pool_size) template template -Page& PagePool::Get(PageId id) { +StatusOrRef PagePool::Get(PageId id) { // Try to locate the page in the pool first. auto pos = pages_to_index_.find(id); if (pos != pages_to_index_.end()) { @@ -155,9 +158,9 @@ Page& PagePool::Get(PageId id) { } // The page is missing, so we need to load it from disk. - auto idx = GetFreeSlot(); + ASSIGN_OR_RETURN(auto idx, GetFreeSlot()); Page& page = pool_[idx].template As(); - file_->LoadPage(id, page); + RETURN_IF_ERROR(file_->LoadPage(id, page)); pages_to_index_[id] = idx; index_to_pages_[idx] = id; eviction_policy_.Read(idx); @@ -187,19 +190,25 @@ void PagePool::AddListener(std::unique_ptr listener) { } template -void PagePool::Flush() { - if (!file_) return; +absl::Status PagePool::Flush() { + if (!file_) { + return absl::OkStatus(); + } for (std::size_t i = 0; i < pool_size_; i++) { if (!dirty_[i]) continue; - file_->StorePage(index_to_pages_[i], pool_[i]); + RETURN_IF_ERROR(file_->StorePage(index_to_pages_[i], pool_[i])); dirty_[i] = false; } + return absl::OkStatus(); } template -void PagePool::Close() { - Flush(); - if (file_) file_->Close(); +absl::Status PagePool::Close() { + RETURN_IF_ERROR(Flush()); + if (file_) { + RETURN_IF_ERROR(file_->Close()); + } + return absl::OkStatus(); } template @@ -215,7 +224,7 @@ MemoryFootprint PagePool::GetMemoryFootprint() const { } template -std::size_t PagePool::GetFreeSlot() { +absl::StatusOr PagePool::GetFreeSlot() { // If there are unused pages, use those first. if (!free_list_.empty()) { std::size_t res = free_list_.back(); @@ -232,12 +241,12 @@ std::size_t PagePool::GetFreeSlot() { } // Evict page to make space. - EvictSlot(*trg); + RETURN_IF_ERROR(EvictSlot(*trg)); return *trg; } template -void PagePool::EvictSlot(int pos) { +absl::Status PagePool::EvictSlot(int pos) { // Notify listeners about pending eviction. auto page_id = index_to_pages_[pos]; bool is_dirty = dirty_[pos]; @@ -247,13 +256,14 @@ void PagePool::EvictSlot(int pos) { // Write to file if dirty. if (is_dirty) { - file_->StorePage(page_id, pool_[pos]); + RETURN_IF_ERROR(file_->StorePage(page_id, pool_[pos])); dirty_[pos] = false; } // Erase page ID association of slot. pages_to_index_.erase(page_id); eviction_policy_.Removed(pos); + return absl::OkStatus(); } } // namespace carmen::backend diff --git a/cpp/backend/common/page_pool_benchmark.cc b/cpp/backend/common/page_pool_benchmark.cc index 5f6fef7cb..19333ec63 100644 --- a/cpp/backend/common/page_pool_benchmark.cc +++ b/cpp/backend/common/page_pool_benchmark.cc @@ -1,11 +1,12 @@ -#include #include +#include "absl/status/status.h" #include "backend/common/access_pattern.h" #include "backend/common/eviction_policy.h" #include "backend/common/page.h" #include "backend/common/page_pool.h" #include "benchmark/benchmark.h" +#include "common/status_test_util.h" namespace carmen::backend { namespace { @@ -23,12 +24,18 @@ class DummyFile { public: constexpr static const std::size_t kPageSize = sizeof(Page); + static absl::StatusOr Open(const std::filesystem::path&) { + return DummyFile(); + } std::size_t GetNumPages() { return kFileSize; } - - void LoadPage(PageId, std::span) {} - void StorePage(PageId, std::span) {} - void Flush() {} - void Close() {} + absl::Status LoadPage(PageId, std::span) { + return absl::OkStatus(); + } + absl::Status StorePage(PageId, std::span) { + return absl::OkStatus(); + } + absl::Status Flush() { return absl::OkStatus(); } + absl::Status Close() { return absl::OkStatus(); } }; template @@ -43,12 +50,12 @@ void BM_ReadTest(benchmark::State& state) { // Warm-up by touching each page once. for (int64_t i = 0; i < pool_size; i++) { - pool.template Get(i); + ASSERT_OK(pool.template Get(i)); } AccessOrder order(kFileSize); for (auto _ : state) { - pool.template Get(order.Next()); + ASSERT_OK(pool.template Get(order.Next())); } } @@ -67,7 +74,7 @@ BENCHMARK(BM_ReadTest) BENCHMARK(BM_ReadTest) ->Range(kMinPoolSize, kMaxPoolSize); -// Evaluates the performance of writing to pages in page pools. +// Evaluates the performance of writing on pages in page pools. template void BM_WriteTest(benchmark::State& state) { auto pool_size = state.range(0); @@ -75,14 +82,14 @@ void BM_WriteTest(benchmark::State& state) { // Warm-up by touching each page once. for (int64_t i = 0; i < pool_size; i++) { - pool.template Get(i); + ASSERT_OK(pool.template Get(i)); pool.MarkAsDirty(i); } AccessOrder order(kFileSize); for (auto _ : state) { auto pos = order.Next(); - pool.template Get(pos); + ASSERT_OK(pool.template Get(pos)); pool.MarkAsDirty(pos); } } diff --git a/cpp/backend/common/page_pool_test.cc b/cpp/backend/common/page_pool_test.cc index fd5186757..1bb3a3e74 100644 --- a/cpp/backend/common/page_pool_test.cc +++ b/cpp/backend/common/page_pool_test.cc @@ -2,10 +2,11 @@ #include #include -#include +#include "absl/status/status.h" #include "backend/common/file.h" #include "backend/common/page.h" +#include "common/status_test_util.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -36,14 +37,14 @@ TEST(PagePoolTest, PoolSizeCanBeDefined) { TEST(PagePoolTest, PagesCanBeFetched) { TestPool pool(2); - auto& page_12 = pool.Get(12); - auto& page_14 = pool.Get(14); + ASSERT_OK_AND_ASSIGN(Page & page_12, pool.Get(12)); + ASSERT_OK_AND_ASSIGN(Page & page_14, pool.Get(14)); EXPECT_NE(&page_12, &page_14); } TEST(PagePoolTest, FreshFetchedPagesAreZeroInitialized) { TestPool pool(2); - auto& page_12 = pool.Get(12); + ASSERT_OK_AND_ASSIGN(Page & page_12, pool.Get(12)); for (int i = 0; i < 4; i++) { EXPECT_EQ(0, page_12[i]); } @@ -56,7 +57,7 @@ TEST(PagePoolTest, PagesAreEvictedAndReloadedCorrectly) { // Write data to kNumSteps pages; for (int i = 0; i < kNumSteps; i++) { - auto& page = pool.Get(i); + ASSERT_OK_AND_ASSIGN(Page & page, pool.Get(i)); page[0] = i; page[1] = i + 1; pool.MarkAsDirty(i); @@ -64,7 +65,7 @@ TEST(PagePoolTest, PagesAreEvictedAndReloadedCorrectly) { // Fetch those kNumSteps pages and check the content for (int i = 0; i < kNumSteps; i++) { - auto& page = pool.Get(i); + ASSERT_OK_AND_ASSIGN(Page & page, pool.Get(i)); EXPECT_EQ(i, page[0]); EXPECT_EQ(i + 1, page[1]); } @@ -94,13 +95,13 @@ TEST(PagePoolTest, ListenersAreNotifiedOnLoad) { EXPECT_CALL(mock, AfterLoad(0, _)).InSequence(s); // Loads page 0 into pool, no eviction. - pool.Get(0); + ASSERT_OK(pool.Get(0)); // Loads page 1 into pool, evicts page 0, which is not dirty. - pool.Get(1); + ASSERT_OK(pool.Get(1)); // Loads page 0 into pool, evicts page 1, which is not dirty. - pool.Get(0); + ASSERT_OK(pool.Get(0)); } TEST(PagePoolTest, ListenersAreNotifiedOnEviction) { @@ -115,25 +116,28 @@ TEST(PagePoolTest, ListenersAreNotifiedOnEviction) { EXPECT_CALL(mock, BeforeEvict(1, _, false)).InSequence(s); // Loads page 0 into pool, no eviction. - pool.Get(0); + ASSERT_OK(pool.Get(0)); // Loads page 1 into pool, evicts page 0, which is not dirty. - pool.Get(1); + ASSERT_OK(pool.Get(1)); // Loads page 0 into pool, evicts page 1, which is not dirty. - pool.Get(0); + ASSERT_OK(pool.Get(0)); } class MockFile { public: constexpr static std::size_t kPageSize = kFileSystemPageSize; + static absl::StatusOr Open(const std::filesystem::path&) { + return absl::StatusOr(); + } MOCK_METHOD(std::size_t, GetNumPages, ()); - MOCK_METHOD(void, LoadPage, + MOCK_METHOD(absl::Status, LoadPage, (PageId id, (std::span dest))); - MOCK_METHOD(void, StorePage, + MOCK_METHOD(absl::Status, StorePage, (PageId id, (std::span src))); - MOCK_METHOD(void, Flush, ()); - MOCK_METHOD(void, Close, ()); + MOCK_METHOD(absl::Status, Flush, ()); + MOCK_METHOD(absl::Status, Close, ()); }; TEST(MockFileTest, IsFile) { EXPECT_TRUE(File); } @@ -148,12 +152,12 @@ TEST(PagePoolTest, FlushWritesDirtyPages) { EXPECT_CALL(mock, StorePage(10, _)); EXPECT_CALL(mock, StorePage(20, _)); - pool.Get(10); - pool.Get(20); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); pool.MarkAsDirty(10); pool.MarkAsDirty(20); - pool.Flush(); + ASSERT_OK(pool.Flush()); } TEST(PagePoolTest, FlushResetsPageState) { @@ -164,11 +168,11 @@ TEST(PagePoolTest, FlushResetsPageState) { EXPECT_CALL(mock, LoadPage(10, _)); EXPECT_CALL(mock, StorePage(10, _)); - pool.Get(10); + ASSERT_OK(pool.Get(10)); pool.MarkAsDirty(10); - pool.Flush(); - pool.Flush(); // < not written a second time + ASSERT_OK(pool.Flush()); + ASSERT_OK(pool.Flush()); // < not written a second time } TEST(PagePoolTest, CleanPagesAreNotFlushed) { @@ -180,11 +184,11 @@ TEST(PagePoolTest, CleanPagesAreNotFlushed) { EXPECT_CALL(mock, LoadPage(20, _)); EXPECT_CALL(mock, StorePage(20, _)); - pool.Get(10); - pool.Get(20); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); pool.MarkAsDirty(20); - pool.Flush(); + ASSERT_OK(pool.Flush()); } TEST(PagePoolTest, ClosingPoolFlushesPagesAndClosesFile) { @@ -197,11 +201,11 @@ TEST(PagePoolTest, ClosingPoolFlushesPagesAndClosesFile) { EXPECT_CALL(mock, StorePage(20, _)); EXPECT_CALL(mock, Close()); - pool.Get(10); - pool.Get(20); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); pool.MarkAsDirty(20); - pool.Close(); + ASSERT_OK(pool.Close()); } class MockEvictionPolicy { @@ -227,9 +231,9 @@ TEST(PagePoolTest, EvictionPolicyIsInformedAboutRead) { EXPECT_CALL(mock, Read(1)).InSequence(s); EXPECT_CALL(mock, Read(0)).InSequence(s); - pool.Get(10); - pool.Get(20); - pool.Get(10); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); + ASSERT_OK(pool.Get(10)); } TEST(PagePoolTest, EvictionPolicyIsInformedAboutWrite) { @@ -245,9 +249,9 @@ TEST(PagePoolTest, EvictionPolicyIsInformedAboutWrite) { EXPECT_CALL(mock, Written(1)); } - pool.Get(10); + ASSERT_OK(pool.Get(10)); pool.MarkAsDirty(10); - pool.Get(20); + ASSERT_OK(pool.Get(20)); pool.MarkAsDirty(20); } @@ -268,10 +272,10 @@ TEST(PagePoolTest, OnEvictionPolicyIsConsultedAndInformed) { EXPECT_CALL(mock, Read(0)); } - pool.Get(10); - pool.Get(20); - pool.Get(30); - pool.Get(40); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); + ASSERT_OK(pool.Get(30)); + ASSERT_OK(pool.Get(40)); } TEST(PagePoolTest, OnFallBackEvictionPolicyIsInformed) { @@ -288,9 +292,9 @@ TEST(PagePoolTest, OnFallBackEvictionPolicyIsInformed) { EXPECT_CALL(mock, Read(_)); } - pool.Get(10); - pool.Get(20); - pool.Get(30); + ASSERT_OK(pool.Get(10)); + ASSERT_OK(pool.Get(20)); + ASSERT_OK(pool.Get(30)); } } // namespace diff --git a/cpp/backend/depot/BUILD b/cpp/backend/depot/BUILD index b9af2286e..b6d63da2d 100644 --- a/cpp/backend/depot/BUILD +++ b/cpp/backend/depot/BUILD @@ -19,6 +19,8 @@ cc_library( "//backend/depot/cache", "//backend/depot/memory:depot", "//common:file_util", + "//common:status_util", + "@com_google_absl//absl/status:statusor", ], ) @@ -61,6 +63,7 @@ cc_binary( "//backend/depot/leveldb:depot", "//backend/depot/memory:depot", "//common:benchmark", + "//common:status_test_util", "//third_party/gperftools:profiler", "@com_github_google_benchmark//:benchmark_main", ], diff --git a/cpp/backend/depot/cache/cache_test.cc b/cpp/backend/depot/cache/cache_test.cc index 7d2f32ed7..5074970cf 100644 --- a/cpp/backend/depot/cache/cache_test.cc +++ b/cpp/backend/depot/cache/cache_test.cc @@ -15,6 +15,7 @@ namespace { using ::testing::_; using ::testing::ElementsAreArray; +using ::testing::IsOkAndHolds; using ::testing::Return; using ::testing::StatusIs; @@ -33,10 +34,8 @@ TEST(CachedDepot, CachedKeysAreNotFetched) { EXPECT_CALL(mock, Get(10)) .WillOnce(Return(absl::StatusOr>(val))); - ASSERT_OK_AND_ASSIGN(auto result, depot.Get(10)); - EXPECT_THAT(result, ElementsAreArray(val)); - ASSERT_OK_AND_ASSIGN(result, depot.Get(10)); - EXPECT_THAT(result, ElementsAreArray(val)); + EXPECT_THAT(depot.Get(10), IsOkAndHolds(ElementsAreArray(val))); + EXPECT_THAT(depot.Get(10), IsOkAndHolds(ElementsAreArray(val))); } TEST(CachedDepot, MissingEntriesAreCached) { @@ -62,10 +61,8 @@ TEST(CachedDepot, HashesAreCached) { Hash hash{0x01, 0x23}; EXPECT_CALL(mock, GetHash()).WillOnce(Return(absl::StatusOr(hash))); - ASSERT_OK_AND_ASSIGN(auto result, depot.GetHash()); - EXPECT_EQ(hash, result); - ASSERT_OK_AND_ASSIGN(result, depot.GetHash()); - EXPECT_EQ(hash, result); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash)); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash)); } TEST(CachedDepot, AddNewElementInvalidatesHash) { @@ -82,18 +79,14 @@ TEST(CachedDepot, AddNewElementInvalidatesHash) { .WillOnce(Return(absl::StatusOr(hash_a))) .WillOnce(Return(absl::StatusOr(hash_b))); - ASSERT_OK_AND_ASSIGN(auto result, depot.GetHash()); - EXPECT_EQ(hash_a, result); - ASSERT_OK_AND_ASSIGN(result, depot.GetHash()); - EXPECT_EQ(hash_a, result); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash_a)); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash_a)); EXPECT_CALL(mock, Set(10, _)).WillOnce(Return(absl::OkStatus())); ASSERT_OK(depot.Set(10, val)); - ASSERT_OK_AND_ASSIGN(result, depot.GetHash()); - EXPECT_EQ(hash_b, result); - ASSERT_OK_AND_ASSIGN(result, depot.GetHash()); - EXPECT_EQ(hash_b, result); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash_b)); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash_b)); } TEST(CachedDepot, CacheSizeLimitIsEnforced) { diff --git a/cpp/backend/depot/depot_benchmark.cc b/cpp/backend/depot/depot_benchmark.cc index ea248ff5f..a09bbddca 100644 --- a/cpp/backend/depot/depot_benchmark.cc +++ b/cpp/backend/depot/depot_benchmark.cc @@ -7,6 +7,7 @@ #include "backend/depot/memory/depot.h" #include "benchmark/benchmark.h" #include "common/benchmark.h" +#include "common/status_test_util.h" namespace carmen::backend::depot { namespace { @@ -40,12 +41,13 @@ void InitDepot(Depot& depot, std::size_t num_elements) { // Benchmarks the sequential insertion of keys into depots. template void BM_SequentialInsert(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); for (auto _ : state) { - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); auto& depot = wrapper.GetDepot(); for (int i = 0; i < num_elements; i++) { - auto res = depot.Set(i, kInsertValue); + ASSERT_OK(depot.Set(i, kInsertValue)); } } } @@ -55,18 +57,19 @@ BENCHMARK_ALL(BM_SequentialInsert, DepotConfigList)->ArgList(kSizes); // Benchmarks the appending of new elements to the depot. template void BM_Insert(benchmark::State& state) { + using Handler = DepotHandler; // The size of the depot before the inserts. auto num_elements = state.range(0); // Initialize the depot with the initial number of elements. - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); auto& depot = wrapper.GetDepot(); InitDepot(depot, num_elements); // Append additional elements to the end of the depot. auto i = num_elements; for (auto _ : state) { - auto res = depot.Set(i++, kInsertValue); + ASSERT_OK(depot.Set(i++, kInsertValue)); } } @@ -75,8 +78,9 @@ BENCHMARK_ALL(BM_Insert, DepotConfigList)->ArgList(kSizes); // Benchmarks sequential read of keys. template void BM_SequentialRead(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -84,7 +88,7 @@ void BM_SequentialRead(benchmark::State& state) { int i = 0; for (auto _ : state) { - auto value = depot.Get(i++ % num_elements); + ASSERT_OK_AND_ASSIGN(auto value, depot.Get(i++ % num_elements)); benchmark::DoNotOptimize(value); } } @@ -94,8 +98,9 @@ BENCHMARK_ALL(BM_SequentialRead, DepotConfigList)->ArgList(kSizes); // Benchmarks random, uniformly distributed reads template void BM_UniformRandomRead(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -105,7 +110,7 @@ void BM_UniformRandomRead(benchmark::State& state) { std::mt19937 gen(rd()); std::uniform_int_distribution<> dist(0, num_elements - 1); for (auto _ : state) { - auto value = depot.Get(dist(gen)); + ASSERT_OK_AND_ASSIGN(auto value, depot.Get(dist(gen))); benchmark::DoNotOptimize(value); } } @@ -115,8 +120,9 @@ BENCHMARK_ALL(BM_UniformRandomRead, DepotConfigList)->ArgList(kSizes); // Benchmarks random, exponentially distributed reads template void BM_ExponentialRandomRead(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -126,7 +132,9 @@ void BM_ExponentialRandomRead(benchmark::State& state) { std::mt19937 gen(rd()); std::exponential_distribution<> dist(double(10) / num_elements); for (auto _ : state) { - auto value = depot.Get(static_cast(dist(gen)) % num_elements); + ASSERT_OK_AND_ASSIGN( + auto value, + depot.Get(static_cast(dist(gen)) % num_elements)); benchmark::DoNotOptimize(value); } } @@ -136,8 +144,9 @@ BENCHMARK_ALL(BM_ExponentialRandomRead, DepotConfigList)->ArgList(kSizes); // Benchmarks sequential writes of keys. template void BM_SequentialWrite(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -146,17 +155,18 @@ void BM_SequentialWrite(benchmark::State& state) { int i = 0; for (auto _ : state) { auto value = std::array{static_cast(i)}; - auto res = depot.Set(i++ % num_elements, value); + ASSERT_OK(depot.Set(i++ % num_elements, value)); } } BENCHMARK_ALL(BM_SequentialWrite, DepotConfigList)->ArgList(kSizes); -// Benchmarks random, uniformely distributed writes. +// Benchmarks random, uniformly distributed writes. template void BM_UniformRandomWrite(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -168,7 +178,7 @@ void BM_UniformRandomWrite(benchmark::State& state) { std::uniform_int_distribution<> dist(0, num_elements - 1); for (auto _ : state) { auto value = std::array{static_cast(i)}; - auto res = depot.Set(dist(gen), value); + ASSERT_OK(depot.Set(dist(gen), value)); } } @@ -177,8 +187,9 @@ BENCHMARK_ALL(BM_UniformRandomWrite, DepotConfigList)->ArgList(kSizes); // Benchmarks sequential read of keys. template void BM_ExponentialRandomWrite(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -190,7 +201,7 @@ void BM_ExponentialRandomWrite(benchmark::State& state) { std::exponential_distribution<> dist(double(10) / num_elements); for (auto _ : state) { auto value = std::array{static_cast(i)}; - auto res = depot.Set(dist(gen), value); + ASSERT_OK(depot.Set(dist(gen), value)); } } @@ -198,8 +209,9 @@ BENCHMARK_ALL(BM_ExponentialRandomWrite, DepotConfigList)->ArgList(kSizes); template void RunHashSequentialUpdates(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -213,11 +225,11 @@ void RunHashSequentialUpdates(benchmark::State& state) { auto value = std::array{ static_cast(i >> 24), static_cast(i >> 16), static_cast(i >> 8), static_cast(i)}; - auto res = depot.Set(i++ % num_elements, value); + ASSERT_OK(depot.Set(i++ % num_elements, value)); } if (!include_write_time) state.ResumeTiming(); - auto hash = depot.GetHash(); + ASSERT_OK_AND_ASSIGN(auto hash, depot.GetHash()); benchmark::DoNotOptimize(hash); } } @@ -231,8 +243,9 @@ BENCHMARK_ALL(BM_HashSequentialUpdates, DepotConfigList)->ArgList(kSizes); template void RunHashUniformUpdates(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -250,11 +263,11 @@ void RunHashUniformUpdates(benchmark::State& state) { static_cast(i >> 24), static_cast(i >> 16), static_cast(i >> 8), static_cast(i)}; i++; - auto res = depot.Set(dist(gen), value); + ASSERT_OK(depot.Set(dist(gen), value)); } if (!include_write_time) state.ResumeTiming(); - auto hash = depot.GetHash(); + ASSERT_OK_AND_ASSIGN(auto hash, depot.GetHash()); benchmark::DoNotOptimize(hash); } } @@ -268,8 +281,9 @@ BENCHMARK_ALL(BM_HashUniformUpdates, DepotConfigList)->ArgList(kSizes); template void RunHashExponentialUpdates(benchmark::State& state) { + using Handler = DepotHandler; auto num_elements = state.range(0); - DepotHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the depot with the total number of elements. auto& depot = wrapper.GetDepot(); @@ -287,11 +301,11 @@ void RunHashExponentialUpdates(benchmark::State& state) { static_cast(i >> 24), static_cast(i >> 16), static_cast(i >> 8), static_cast(i)}; i++; - auto res = depot.Set(std::size_t(dist(gen)) % num_elements, value); + ASSERT_OK(depot.Set(std::size_t(dist(gen)) % num_elements, value)); } if (!include_write_time) state.ResumeTiming(); - auto hash = depot.GetHash(); + ASSERT_OK_AND_ASSIGN(auto hash, depot.GetHash()); benchmark::DoNotOptimize(hash); } } diff --git a/cpp/backend/depot/depot_handler.h b/cpp/backend/depot/depot_handler.h index 4f66759de..a2acdb977 100644 --- a/cpp/backend/depot/depot_handler.h +++ b/cpp/backend/depot/depot_handler.h @@ -2,10 +2,12 @@ #include +#include "absl/status/statusor.h" #include "backend/depot/cache/cache.h" #include "backend/depot/depot.h" #include "backend/depot/memory/depot.h" #include "common/file_util.h" +#include "common/status_util.h" namespace carmen::backend::depot { namespace { @@ -29,10 +31,7 @@ class DepotHandlerBase { auto& GetReferenceDepot() { return reference_; } - std::filesystem::path GetDepotDirectory() const { return temp_dir_; } - private: - TempDir temp_dir_; ReferenceDepot reference_; }; @@ -47,43 +46,48 @@ template class DepotHandler : public DepotHandlerBase { public: - using DepotHandlerBase::GetDepotDirectory; - DepotHandler() - : depot_(*Depot::Open(GetDepotDirectory(), branching_factor, - hash_box_size)) {} + template + static absl::StatusOr Create(Args&&... args) { + TempDir dir; + ASSIGN_OR_RETURN(auto depot, + Depot::Open(dir.GetPath(), branching_factor, hash_box_size, + std::forward(args)...)); + return DepotHandler(std::move(depot), std::move(dir)); + } + Depot& GetDepot() { return depot_; } private: - Depot depot_; -}; + DepotHandler(Depot depot, TempDir dir) + : temp_dir_(std::move(dir)), depot_(std::move(depot)) {} -// A specialization of a DepotHandler for InMemoryDepot handling ignoring the -// creation/deletion of temporary files and directories. -template -class DepotHandler, branching_factor, hash_box_size> - : public DepotHandlerBase { - public: - DepotHandler() : depot_(branching_factor, hash_box_size) {} - InMemoryDepot& GetDepot() { return depot_; } - - private: - InMemoryDepot depot_; + TempDir temp_dir_; + Depot depot_; }; // A specialization of a DepotHandler for Cached depot handling ignoring the // creation/deletion of temporary files and directories. -template -class DepotHandler, branching_factor, num_hash_boxes> +template +class DepotHandler, branching_factor, hash_box_size> : public DepotHandlerBase { + hash_box_size> { public: - DepotHandler() : depot_(std::move(nested_.GetDepot())) {} + template + static absl::StatusOr Create(Args&&... args) { + TempDir dir; + ASSIGN_OR_RETURN(auto depot, + Depot::Open(dir.GetPath(), branching_factor, hash_box_size, + std::forward(args)...)); + return DepotHandler(std::move(depot), std::move(dir)); + } + Cached& GetDepot() { return depot_; } private: - DepotHandler nested_; + DepotHandler(Depot nested, TempDir dir) + : temp_dir_(std::move(dir)), depot_(std::move(nested)) {} + + TempDir temp_dir_; Cached depot_; }; diff --git a/cpp/backend/depot/depot_test.cc b/cpp/backend/depot/depot_test.cc index adf1d4776..4420c3dd0 100644 --- a/cpp/backend/depot/depot_test.cc +++ b/cpp/backend/depot/depot_test.cc @@ -27,13 +27,13 @@ class DepotTest : public testing::Test {}; TYPED_TEST_SUITE_P(DepotTest); TYPED_TEST_P(DepotTest, TypeProperties) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_TRUE(std::is_move_constructible_v); } TYPED_TEST_P(DepotTest, DataCanBeAddedAndRetrieved) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_THAT(depot.Get(10), StatusIs(absl::StatusCode::kNotFound, _)); @@ -50,7 +50,7 @@ TYPED_TEST_P(DepotTest, DataCanBeAddedAndRetrieved) { } TYPED_TEST_P(DepotTest, EntriesCanBeUpdated) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_OK(depot.Set(10, std::array{std::byte{1}, std::byte{2}})); @@ -64,7 +64,7 @@ TYPED_TEST_P(DepotTest, EntriesCanBeUpdated) { } TYPED_TEST_P(DepotTest, SizeCanBeFatched) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_THAT(depot.GetSize(10), StatusIs(absl::StatusCode::kNotFound, _)); @@ -74,13 +74,13 @@ TYPED_TEST_P(DepotTest, SizeCanBeFatched) { } TYPED_TEST_P(DepotTest, EmptyDepotHasZeroHash) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_THAT(depot.GetHash(), IsOkAndHolds(Hash{})); } TYPED_TEST_P(DepotTest, NonEmptyDepotHasHash) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); ASSERT_OK_AND_ASSIGN(auto initial_hash, depot.GetHash()); @@ -90,7 +90,7 @@ TYPED_TEST_P(DepotTest, NonEmptyDepotHasHash) { } TYPED_TEST_P(DepotTest, HashChangesBack) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_OK(depot.Set(10, std::array{std::byte{1}, std::byte{2}})); @@ -115,7 +115,7 @@ TYPED_TEST_P(DepotTest, KnownHashesAreReproduced) { "of 2."; } - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); // Tests the hashes for values [0x00], [0x00, 0x11] ... [..., 0xFF] inserted @@ -151,7 +151,7 @@ TYPED_TEST_P(DepotTest, KnownHashesAreReproduced) { } TYPED_TEST_P(DepotTest, EmptyCodeCanBeStored) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); EXPECT_OK(depot.Set(10, std::span{})); EXPECT_THAT(depot.Get(10), IsOkAndHolds(IsEmpty())); @@ -159,7 +159,7 @@ TYPED_TEST_P(DepotTest, EmptyCodeCanBeStored) { TYPED_TEST_P(DepotTest, HashesEqualReferenceImplementation) { constexpr int N = 100; - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& depot = wrapper.GetDepot(); auto& reference = wrapper.GetReferenceDepot(); diff --git a/cpp/backend/depot/file/depot.h b/cpp/backend/depot/file/depot.h index 15082dbc8..d241afc26 100644 --- a/cpp/backend/depot/file/depot.h +++ b/cpp/backend/depot/file/depot.h @@ -39,10 +39,7 @@ class FileDepot { std::size_t hash_branching_factor = 32, std::size_t hash_box_size = 4) { // Make sure the parent directory exists. - if (!CreateDirectory(path)) { - return absl::InternalError( - absl::StrFormat("Unable to create parent directory %s", path)); - } + RETURN_IF_ERROR(CreateDirectory(path)); auto offset_file = path / "offset.dat"; auto data_file = path / "data.dat"; diff --git a/cpp/backend/depot/file/depot_test.cc b/cpp/backend/depot/file/depot_test.cc index 2c820d716..4db198011 100644 --- a/cpp/backend/depot/file/depot_test.cc +++ b/cpp/backend/depot/file/depot_test.cc @@ -30,18 +30,15 @@ TEST(FileDepotTest, TestIsPersistent) { { ASSERT_OK_AND_ASSIGN(auto depot, TestDepot::Open(dir.GetPath())); EXPECT_THAT(depot.Get(10), StatusIs(absl::StatusCode::kNotFound, _)); - ASSERT_OK_AND_ASSIGN(auto empty_hash, depot.GetHash()); - EXPECT_EQ(empty_hash, Hash{}); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(Hash{})); ASSERT_OK(depot.Set(10, elements)); ASSERT_OK_AND_ASSIGN(hash, depot.GetHash()); } { ASSERT_OK_AND_ASSIGN(auto depot, TestDepot::Open(dir.GetPath())); - ASSERT_OK_AND_ASSIGN(auto val, depot.Get(10)); - EXPECT_THAT(val, ElementsAreArray(elements)); - ASSERT_OK_AND_ASSIGN(auto new_hash, depot.GetHash()); - EXPECT_EQ(new_hash, hash); + EXPECT_THAT(depot.Get(10), IsOkAndHolds(ElementsAreArray(elements))); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash)); } } diff --git a/cpp/backend/depot/leveldb/depot_test.cc b/cpp/backend/depot/leveldb/depot_test.cc index c019df1c5..ba5140c81 100644 --- a/cpp/backend/depot/leveldb/depot_test.cc +++ b/cpp/backend/depot/leveldb/depot_test.cc @@ -12,6 +12,7 @@ namespace { using ::testing::_; using ::testing::ElementsAreArray; +using ::testing::IsOkAndHolds; using ::testing::StatusIs; using TestDepot = LevelDbDepot; @@ -26,18 +27,15 @@ TEST(LevelDbDepotTest, TestIsPersistent) { { ASSERT_OK_AND_ASSIGN(auto depot, TestDepot::Open(dir.GetPath())); EXPECT_THAT(depot.Get(10), StatusIs(absl::StatusCode::kNotFound, _)); - ASSERT_OK_AND_ASSIGN(auto empty_hash, depot.GetHash()); - EXPECT_EQ(empty_hash, Hash{}); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(Hash{})); ASSERT_OK(depot.Set(10, elements)); ASSERT_OK_AND_ASSIGN(hash, depot.GetHash()); } { ASSERT_OK_AND_ASSIGN(auto depot, TestDepot::Open(dir.GetPath())); - ASSERT_OK_AND_ASSIGN(auto val, depot.Get(10)); - EXPECT_THAT(val, ElementsAreArray(elements)); - ASSERT_OK_AND_ASSIGN(auto new_hash, depot.GetHash()); - EXPECT_EQ(new_hash, hash); + EXPECT_THAT(depot.Get(10), IsOkAndHolds(ElementsAreArray(elements))); + EXPECT_THAT(depot.GetHash(), IsOkAndHolds(hash)); } } diff --git a/cpp/backend/depot/memory/depot.h b/cpp/backend/depot/memory/depot.h index 12e12eb4b..8199d326d 100644 --- a/cpp/backend/depot/memory/depot.h +++ b/cpp/backend/depot/memory/depot.h @@ -26,6 +26,12 @@ class InMemoryDepot { return InMemoryDepot(); } + static absl::StatusOr Open( + const std::filesystem::path&, std::size_t hash_branching_factor = 32, + std::size_t hash_box_size = 4) { + return InMemoryDepot(hash_branching_factor, hash_box_size); + } + // Creates a new InMemoryDepot using the provided branching factor and // number of items per group for hash computation. InMemoryDepot(std::size_t hash_branching_factor = 32, diff --git a/cpp/backend/index/BUILD b/cpp/backend/index/BUILD index 51cc11d67..087ad4a4d 100644 --- a/cpp/backend/index/BUILD +++ b/cpp/backend/index/BUILD @@ -59,6 +59,8 @@ cc_library( "//backend/index/file:index", "//backend/index/leveldb/multi_db:index", "//backend/index/leveldb/single_db:index", + "@com_google_absl//absl/status:statusor", + "//backend:structure", "//common:file_util", "//common:type", ], diff --git a/cpp/backend/index/cache/cache_test.cc b/cpp/backend/index/cache/cache_test.cc index abee47b73..ffad94b00 100644 --- a/cpp/backend/index/cache/cache_test.cc +++ b/cpp/backend/index/cache/cache_test.cc @@ -32,9 +32,9 @@ TEST(CachedIndex, CachedKeysAreNotFetched) { .WillOnce( Return(absl::StatusOr>(std::pair{10, true}))); - EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(std::pair(10, true))); - EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(std::pair(10, false))); - EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(std::pair(10, false))); + EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(Pair(10, true))); + EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(Pair(10, false))); + EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(Pair(10, false))); } TEST(CachedIndex, MissingEntriesAreCached) { @@ -139,21 +139,21 @@ TEST(CachedIndex, CacheSizeLimitIsEnforced) { .WillOnce( Return(absl::StatusOr>(std::pair{2, true}))); - EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(std::pair(0, true))); - EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(std::pair(1, true))); + EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(Pair(0, true))); + EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(Pair(1, true))); // At this point keys 1 and 2 are in the cache, we can query them without // reload. - EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(std::pair(0, false))); - EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(std::pair(1, false))); - EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(std::pair(0, false))); - EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(std::pair(1, false))); + EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(Pair(0, false))); + EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(Pair(1, false))); + EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(Pair(0, false))); + EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(Pair(1, false))); // Asking for key=2 will kick out key 0. - EXPECT_THAT(index.GetOrAdd(2), IsOkAndHolds(std::pair(2, true))); + EXPECT_THAT(index.GetOrAdd(2), IsOkAndHolds(Pair(2, true))); // At this point, key=0 is forgotten. This will trigger a second call. - EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(std::pair(0, false))); + EXPECT_THAT(index.GetOrAdd(0), IsOkAndHolds(Pair(0, false))); } } // namespace diff --git a/cpp/backend/index/file/BUILD b/cpp/backend/index/file/BUILD index a7bd1c58b..6ee4c3de9 100644 --- a/cpp/backend/index/file/BUILD +++ b/cpp/backend/index/file/BUILD @@ -64,8 +64,10 @@ cc_library( "//common:memory_usage", "//common:status_util", "//common:type", + "//common:fstream", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", + "@com_google_absl//absl/strings:strings", ], ) @@ -75,6 +77,7 @@ cc_test( deps = [ ":index", "//backend/index:test_util", + "//backend:structure", "//common:status_test_util", "@com_google_googletest//:gtest_main", ], diff --git a/cpp/backend/index/file/hash_page.h b/cpp/backend/index/file/hash_page.h index b0e9678e0..eed8e1948 100644 --- a/cpp/backend/index/file/hash_page.h +++ b/cpp/backend/index/file/hash_page.h @@ -1,8 +1,7 @@ #pragma once -#include - #include +#include #include #include "backend/common/page.h" @@ -125,9 +124,9 @@ class alignas(kFileSystemPageSize) HashPage { // Gets the number of elements in this page. std::size_t Size() const { return GetMetadata().size; } - // Updates the size of this page. If the new size is less then the current + // Updates the size of this page. If the new size is less, then the current // size, entries are dropped. If the new size is larger, the additional - // elements will have a unspecified, yet valid value. + // elements will have an unspecified, yet valid value. void Resize(std::size_t new_size) { assert(0 <= new_size && new_size <= kNumEntries); GetMetadata().size = new_size; diff --git a/cpp/backend/index/file/index.h b/cpp/backend/index/file/index.h index 5437a9541..133c2b6eb 100644 --- a/cpp/backend/index/file/index.h +++ b/cpp/backend/index/file/index.h @@ -10,11 +10,13 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/str_cat.h" #include "backend/common/file.h" #include "backend/common/page_pool.h" #include "backend/index/file/hash_page.h" #include "backend/index/file/stable_hash.h" #include "backend/structure.h" +#include "common/fstream.h" #include "common/hash.h" #include "common/memory_usage.h" #include "common/status_util.h" @@ -59,13 +61,7 @@ class FileIndex { static absl::StatusOr Open(Context&, const std::filesystem::path& directory); - // Creates a new, empty index backed by a default-constructed file. - FileIndex(); - - // Creates an index retaining its data in the given directory. - FileIndex(std::filesystem::path directory); - - // File indexes are move-constructible. + // File indexes are move-constructable. FileIndex(FileIndex&&) = default; // On destruction file indexes are automatically flushed and closed. @@ -114,26 +110,27 @@ class FileIndex { // Creates an index based on the given files. FileIndex(std::unique_ptr primary_page_file, std::unique_ptr overflow_page_file, - std::filesystem::path metadata_file = ""); + std::unique_ptr metadata_file); // A helper function to locate an entry in this map. Returns a tuple // containing the key's hash, the containing bucket, and the containing entry. // Only if the entry pointer is not-null the entry has been found. - std::tuple FindInternal( + absl::StatusOr> FindInternal( const K& key) const; // Same as above, but for non-const instances. - std::tuple FindInternal(const K& key); + absl::StatusOr> FindInternal( + const K& key); // Splits one bucket in the hash table causing the table to grow by one // bucket. - void Split(); + absl::Status Split(); // Obtains the index of the bucket the given hash key is supposed to be // located in. - bucket_id_t GetBucket(hash_t hashkey) const { - bucket_id_t bucket = hashkey & high_mask_; - return bucket >= num_buckets_ ? hashkey & low_mask_ : bucket; + bucket_id_t GetBucket(hash_t hash_key) const { + bucket_id_t bucket = hash_key & high_mask_; + return bucket >= num_buckets_ ? hash_key & low_mask_ : bucket; } // Returns the overflow page being the tail fo the given bucket. Returns @@ -231,77 +228,69 @@ template class F, absl::StatusOr> FileIndex::Open(Context&, const std::filesystem::path& directory) { - // TODO: move directory initialization from constructor to factory and do - // proper error handling. - return FileIndex(directory); -} - -template class F, - std::size_t page_size> -FileIndex::FileIndex() - : FileIndex(std::make_unique(), std::make_unique()) {} + ASSIGN_OR_RETURN(auto primary_page_file, + File::Open(directory / "primary.dat")); + ASSIGN_OR_RETURN(auto overflow_page_file, + File::Open(directory / "overflow.dat")); + + auto index = FileIndex( + std::make_unique(std::move(primary_page_file)), + std::make_unique(std::move(overflow_page_file)), + std::make_unique(directory / "metadata.dat")); + if (!std::filesystem::exists(*index.metadata_file_)) { + return index; + } -template class F, - std::size_t page_size> -FileIndex::FileIndex(std::filesystem::path directory) - : FileIndex(std::make_unique(directory / "primary.dat"), - std::make_unique(directory / "overflow.dat"), - directory / "metadata.dat") {} - -template class F, - std::size_t page_size> -FileIndex::FileIndex( - std::unique_ptr primary_page_file, - std::unique_ptr overflow_page_file, - std::filesystem::path metadata_file) - : primary_pool_(std::move(primary_page_file)), - overflow_pool_(std::move(overflow_page_file)), - low_mask_((1 << kInitialHashLength) - 1), - high_mask_((low_mask_ << 1) | 0x1), - num_buckets_(1 << kInitialHashLength) { - // Take ownership of the metadata file and track it through a unique ptr. - metadata_file_ = - std::make_unique(std::move(metadata_file)); - if (!std::filesystem::exists(*metadata_file_)) return; - - // Load metadata from file. - std::fstream in(*metadata_file_, std::ios::binary | std::ios::in); - auto read_scalar = [&](auto& scalar) { - in.read(reinterpret_cast(&scalar), sizeof(scalar)); - }; + ASSIGN_OR_RETURN(auto in, FStream::Open(*index.metadata_file_, + std::ios::binary | std::ios::in)); // Start with scalars. - read_scalar(size_); - read_scalar(next_to_split_); - read_scalar(low_mask_); - read_scalar(high_mask_); - read_scalar(num_buckets_); - read_scalar(num_overflow_pages_); - read_scalar(hash_); + RETURN_IF_ERROR(in.Read(index.size_)); + RETURN_IF_ERROR(in.Read(index.next_to_split_)); + RETURN_IF_ERROR(in.Read(index.low_mask_)); + RETURN_IF_ERROR(in.Read(index.high_mask_)); + RETURN_IF_ERROR(in.Read(index.num_buckets_)); + RETURN_IF_ERROR(in.Read(index.num_overflow_pages_)); + RETURN_IF_ERROR(in.Read(index.hash_)); // Read bucket tail list. - assert(sizeof(bucket_tails_.size()) == sizeof(std::size_t)); + assert(sizeof(index.bucket_tails_.size()) == sizeof(std::size_t)); std::size_t size; - read_scalar(size); - bucket_tails_.resize(size); + RETURN_IF_ERROR(in.Read(size)); + index.bucket_tails_.resize(size); for (std::size_t i = 0; i < size; i++) { - read_scalar(bucket_tails_[i]); + RETURN_IF_ERROR(in.Read(index.bucket_tails_[i])); } // Read free list. - assert(sizeof(overflow_page_free_list_.size()) == sizeof(std::size_t)); - read_scalar(size); - overflow_page_free_list_.resize(size); + assert(sizeof(index.overflow_page_free_list_.size()) == sizeof(std::size_t)); + RETURN_IF_ERROR(in.Read(size)); + index.overflow_page_free_list_.resize(size); for (std::size_t i = 0; i < size; i++) { - read_scalar(overflow_page_free_list_[i]); + RETURN_IF_ERROR(in.Read(index.overflow_page_free_list_[i])); } + + return index; } +template class F, + std::size_t page_size> +FileIndex::FileIndex( + std::unique_ptr primary_page_file, + std::unique_ptr overflow_page_file, + std::unique_ptr metadata_file) + : primary_pool_(std::move(primary_page_file)), + overflow_pool_(std::move(overflow_page_file)), + metadata_file_(std::move(metadata_file)), + low_mask_((1 << kInitialHashLength) - 1), + high_mask_((low_mask_ << 1) | 0x1), + num_buckets_(1 << kInitialHashLength) {} + template class F, std::size_t page_size> absl::StatusOr> FileIndex::GetOrAdd( const K& key) { - auto [hash, bucket, entry] = FindInternal(key); + ASSIGN_OR_RETURN((auto [hash, bucket, entry]), FindInternal(key)); if (entry != nullptr) { return std::pair{entry->value, false}; } @@ -310,7 +299,7 @@ absl::StatusOr> FileIndex::GetOrAdd( // Trigger a split if the bucket has an overflow bucket. if (GetTail(bucket) != kNullPage) { - Split(); + RETURN_IF_ERROR(Split()); // After the split, the target bucket may be a different one. bucket = GetBucket(hash); @@ -320,17 +309,18 @@ absl::StatusOr> FileIndex::GetOrAdd( Page* page; auto tail = GetTail(bucket); if (tail == kNullPage) { - page = &primary_pool_.template Get(bucket); + ASSIGN_OR_RETURN(page, primary_pool_.template Get(bucket)); primary_pool_.MarkAsDirty(bucket); } else { - page = &overflow_pool_.template Get(tail); + ASSIGN_OR_RETURN(page, overflow_pool_.template Get(tail)); overflow_pool_.MarkAsDirty(tail); } if (page->Insert(hash, key, size_ - 1) == nullptr) { auto new_overflow_id = GetFreeOverflowPageId(); page->SetNext(new_overflow_id); - auto overflow_page = &overflow_pool_.template Get(new_overflow_id); + ASSIGN_OR_RETURN(Page * overflow_page, + overflow_pool_.template Get(new_overflow_id)); assert(overflow_page->Size() == 0); assert(overflow_page->GetNext() == 0); SetTail(bucket, new_overflow_id); @@ -345,7 +335,7 @@ absl::StatusOr> FileIndex::GetOrAdd( template class F, std::size_t page_size> absl::StatusOr FileIndex::Get(const K& key) const { - auto [hash, bucket, entry] = FindInternal(key); + ASSIGN_OR_RETURN((auto [hash, bucket, entry]), FindInternal(key)); if (entry == nullptr) { return absl::NotFoundError("Key not found."); } @@ -365,38 +355,36 @@ absl::StatusOr FileIndex::GetHash() const { template class F, std::size_t page_size> absl::Status FileIndex::Flush() { - primary_pool_.Flush(); - overflow_pool_.Flush(); + RETURN_IF_ERROR(primary_pool_.Flush()); + RETURN_IF_ERROR(overflow_pool_.Flush()); // Flush metadata if this is an owning instance. if (!metadata_file_ || metadata_file_->empty()) return absl::OkStatus(); // Sync out metadata information. - std::fstream out(*metadata_file_, std::ios::binary | std::ios::out); - auto write_scalar = [&](auto scalar) { - out.write(reinterpret_cast(&scalar), sizeof(scalar)); - }; + ASSIGN_OR_RETURN(auto out, FStream::Open(*metadata_file_, + std::ios::binary | std::ios::out)); // Start with scalars. - write_scalar(size_); - write_scalar(next_to_split_); - write_scalar(low_mask_); - write_scalar(high_mask_); - write_scalar(num_buckets_); - write_scalar(num_overflow_pages_); + RETURN_IF_ERROR(out.Write(size_)); + RETURN_IF_ERROR(out.Write(next_to_split_)); + RETURN_IF_ERROR(out.Write(low_mask_)); + RETURN_IF_ERROR(out.Write(high_mask_)); + RETURN_IF_ERROR(out.Write(num_buckets_)); + RETURN_IF_ERROR(out.Write(num_overflow_pages_)); ASSIGN_OR_RETURN(auto hash, GetHash()); - write_scalar(hash); + RETURN_IF_ERROR(out.Write(hash)); // Write bucket tail list. - write_scalar(bucket_tails_.size()); + RETURN_IF_ERROR(out.Write(bucket_tails_.size())); for (const auto& page_id : bucket_tails_) { - write_scalar(page_id); + RETURN_IF_ERROR(out.Write(page_id)); } // Write free list. - write_scalar(overflow_page_free_list_.size()); + RETURN_IF_ERROR(out.Write(overflow_page_free_list_.size())); for (const auto& page_id : overflow_page_free_list_) { - write_scalar(page_id); + RETURN_IF_ERROR(out.Write(page_id)); } return absl::OkStatus(); } @@ -405,8 +393,8 @@ template class F, std::size_t page_size> absl::Status FileIndex::Close() { RETURN_IF_ERROR(Flush()); - primary_pool_.Close(); - overflow_pool_.Close(); + RETURN_IF_ERROR(primary_pool_.Close()); + RETURN_IF_ERROR(overflow_pool_.Close()); return absl::OkStatus(); } @@ -418,11 +406,21 @@ void FileIndex::Dump() const { << num_buckets_ << " buckets\n"; for (std::size_t i = 0; i < num_buckets_; i++) { std::cout << "\tBucket " << i << ":\n"; - Page* page = &primary_pool_.template Get(i); + auto result = primary_pool_.template Get(i); + if (!result.ok()) { + std::cout << "\t\tError: " << result.status() << "\n"; + continue; + } + Page* page = result->AsPointer(); while (page != nullptr) { page->Dump(); auto next = page->GetNext(); - page = next == 0 ? nullptr : &overflow_pool_.template Get(next); + result = overflow_pool_.template Get(next); + if (!result.ok()) { + std::cout << "\t\tError: " << result.status() << "\n"; + break; + } + page = next == 0 ? nullptr : result.AsPointer(); } } } @@ -440,41 +438,42 @@ MemoryFootprint FileIndex::GetMemoryFootprint() const { template class F, std::size_t page_size> -std::tuple::hash_t, - typename FileIndex::bucket_id_t, - const typename FileIndex::Entry*> +absl::StatusOr::hash_t, + typename FileIndex::bucket_id_t, + const typename FileIndex::Entry*>> FileIndex::FindInternal(const K& key) const { auto hash = key_hasher_(key); auto bucket = GetBucket(hash); // Search within that bucket. - Page* cur = &primary_pool_.template Get(bucket); + ASSIGN_OR_RETURN(Page * cur, primary_pool_.template Get(bucket)); while (cur != nullptr) { if (auto entry = cur->Find(hash, key)) { - return {hash, bucket, entry}; + return std::tuple{hash, bucket, entry}; } PageId next = cur->GetNext(); - cur = next != 0 ? &overflow_pool_.template Get(next) : nullptr; + ASSIGN_OR_RETURN(cur, overflow_pool_.template Get(next)); + cur = next != 0 ? cur : nullptr; } - // Report a nullpointer if nothing was found. - return {hash, bucket, nullptr}; + // Report a null pointer if nothing was found. + return std::tuple{hash, bucket, nullptr}; } template class F, std::size_t page_size> -std::tuple::hash_t, - typename FileIndex::bucket_id_t, - typename FileIndex::Entry*> +absl::StatusOr::hash_t, + typename FileIndex::bucket_id_t, + typename FileIndex::Entry*>> FileIndex::FindInternal(const K& key) { - auto [hash, bucket, entry] = - const_cast(this)->FindInternal(key); - return {hash, bucket, const_cast(entry)}; + ASSIGN_OR_RETURN((auto [hash, bucket, entry]), + const_cast(this)->FindInternal(key)); + return std::tuple{hash, bucket, const_cast(entry)}; } template class F, std::size_t page_size> -void FileIndex::Split() { +absl::Status FileIndex::Split() { assert(next_to_split_ < num_buckets_); // When a full cycle is completed ... @@ -491,14 +490,15 @@ void FileIndex::Split() { // Load data from page to be split into memory. std::deque entries; - Page* page = &primary_pool_.template Get(old_bucket_id); + ASSIGN_OR_RETURN(Page * page, + primary_pool_.template Get(old_bucket_id)); while (page != nullptr) { for (std::size_t i = 0; i < page->Size(); i++) { entries.push_back((*page)[i]); } auto next = page->GetNext(); if (next != 0) { - page = &overflow_pool_.template Get(next); + ASSIGN_OR_RETURN(page, overflow_pool_.template Get(next)); } else { page = nullptr; } @@ -525,7 +525,7 @@ void FileIndex::Split() { std::sort(new_bucket.begin(), new_bucket.end()); // Write old entries into old bucket. - page = &primary_pool_.template Get(old_bucket_id); + ASSIGN_OR_RETURN(page, primary_pool_.template Get(old_bucket_id)); primary_pool_.MarkAsDirty(old_bucket_id); int i = 0; ResetTail(old_bucket_id); @@ -535,7 +535,7 @@ void FileIndex::Split() { page->Resize(Page::kNumEntries); auto next = page->GetNext(); assert(next != 0); - page = &overflow_pool_.template Get(next); + ASSIGN_OR_RETURN(page, overflow_pool_.template Get(next)); overflow_pool_.MarkAsDirty(next); SetTail(old_bucket_id, next); i = 0; @@ -551,7 +551,7 @@ void FileIndex::Split() { if (next != 0) { page->SetNext(0); ReturnOverflowPage(next); - page = &overflow_pool_.template Get(next); + ASSIGN_OR_RETURN(page, overflow_pool_.template Get(next)); page->Resize(0); overflow_pool_.MarkAsDirty(next); } else { @@ -560,7 +560,7 @@ void FileIndex::Split() { } // Write new entries into new bucket. - page = &primary_pool_.template Get(new_bucket_id); + ASSIGN_OR_RETURN(page, primary_pool_.template Get(new_bucket_id)); i = 0; primary_pool_.MarkAsDirty(new_bucket_id); for (const Entry& entry : new_bucket) { @@ -569,7 +569,7 @@ void FileIndex::Split() { page->Resize(Page::kNumEntries); auto next = GetFreeOverflowPageId(); page->SetNext(next); - page = &overflow_pool_.template Get(next); + ASSIGN_OR_RETURN(page, overflow_pool_.template Get(next)); overflow_pool_.MarkAsDirty(next); assert(page->GetNext() == 0); SetTail(new_bucket_id, next); @@ -579,6 +579,8 @@ void FileIndex::Split() { } remaining = new_bucket.size() % Page::kNumEntries; page->Resize(remaining == 0 ? Page::kNumEntries : remaining); + + return absl::OkStatus(); } } // namespace carmen::backend::index diff --git a/cpp/backend/index/file/index_test.cc b/cpp/backend/index/file/index_test.cc index ca2274548..137d727fe 100644 --- a/cpp/backend/index/file/index_test.cc +++ b/cpp/backend/index/file/index_test.cc @@ -2,6 +2,7 @@ #include "backend/common/file.h" #include "backend/index/test_util.h" +#include "backend/structure.h" #include "common/status_test_util.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -21,9 +22,11 @@ INSTANTIATE_TYPED_TEST_SUITE_P(File, IndexTest, TestIndex); TEST(FileIndexTest, FillTest) { constexpr int N = 1000; - TestIndex index; + Context ctx; + TempDir dir; + ASSERT_OK_AND_ASSIGN(auto index, TestIndex::Open(ctx, dir.GetPath())); for (int i = 0; i < N; i++) { - EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(Pair(i, true))); for (int j = 0; j < N; j++) { if (j <= i) { EXPECT_THAT(index.Get(j), IsOkAndHolds(j)) << "Inserted: " << i << "\n"; @@ -38,9 +41,11 @@ TEST(FileIndexTest, FillTest) { TEST(FileIndexTest, FillTest_SmallPages) { using Index = FileIndex; constexpr int N = 1000; - Index index; + Context ctx; + TempDir dir; + ASSERT_OK_AND_ASSIGN(auto index, Index::Open(ctx, dir.GetPath())); for (std::uint32_t i = 0; i < N; i++) { - EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(Pair(i, true))); for (std::uint32_t j = 0; j <= i; j++) { EXPECT_THAT(index.Get(j), IsOkAndHolds(j)) << "Inserted: " << i << "\n"; } @@ -50,9 +55,11 @@ TEST(FileIndexTest, FillTest_SmallPages) { TEST(FileIndexTest, FillTest_LargePages) { using Index = FileIndex; constexpr int N = 1000; - Index index; + Context ctx; + TempDir dir; + ASSERT_OK_AND_ASSIGN(auto index, Index::Open(ctx, dir.GetPath())); for (std::uint32_t i = 0; i < N; i++) { - EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(Pair(i, true))); for (std::uint32_t j = 0; j <= i; j++) { EXPECT_THAT(index.Get(j), IsOkAndHolds(j)) << "Inserted: " << i << "\n"; } @@ -63,9 +70,11 @@ TEST(FileIndexTest, LastInsertedElementIsPresent) { // The last element being missing was observed as a bug during development. // This test is present to prevent this issue from being re-introduced. constexpr int N = 1000000; - TestIndex index; + Context ctx; + TempDir dir; + ASSERT_OK_AND_ASSIGN(auto index, TestIndex::Open(ctx, dir.GetPath())); for (int i = 0; i < N; i++) { - EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i), IsOkAndHolds(Pair(i, true))); EXPECT_THAT(index.Get(i), IsOkAndHolds(i)); } } @@ -74,16 +83,17 @@ TEST(FileIndexTest, StoreCanBeSavedAndRestored) { using Index = FileIndex; const int kNumElements = 100000; TempDir dir; + Context ctx; Hash hash; { - Index index(dir.GetPath()); + ASSERT_OK_AND_ASSIGN(auto index, Index::Open(ctx, dir.GetPath())); for (int i = 0; i < kNumElements; i++) { - EXPECT_THAT(index.GetOrAdd(i + 5), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i + 5), IsOkAndHolds(Pair(i, true))); } ASSERT_OK_AND_ASSIGN(hash, index.GetHash()); } { - Index restored(dir.GetPath()); + ASSERT_OK_AND_ASSIGN(auto restored, Index::Open(ctx, dir.GetPath())); EXPECT_THAT(restored.GetHash(), IsOkAndHolds(hash)); for (int i = 0; i < kNumElements; i++) { EXPECT_THAT(restored.Get(i + 5), IsOkAndHolds(i)); diff --git a/cpp/backend/index/index.h b/cpp/backend/index/index.h index ea654674a..7940ec717 100644 --- a/cpp/backend/index/index.h +++ b/cpp/backend/index/index.h @@ -10,7 +10,7 @@ namespace carmen::backend::index { // A snapshot of the state of an index providing access to the contained data -// frozen at it creation time. This definies an interface for index +// frozen at it creation time. This defines an interface for index // implementation specific implementations. // // The life cycle of a snapshot defines the duration of its availability. diff --git a/cpp/backend/index/index_benchmark.cc b/cpp/backend/index/index_benchmark.cc index 97998889c..9a1b670a9 100644 --- a/cpp/backend/index/index_benchmark.cc +++ b/cpp/backend/index/index_benchmark.cc @@ -52,7 +52,7 @@ Key ToKey(std::int64_t value) { template void BM_Insert(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); - IndexHandler handler; + ASSERT_OK_AND_ASSIGN(auto handler, IndexHandler::Create()); auto& index = handler.GetIndex(); // Fill in initial elements. @@ -72,7 +72,7 @@ BENCHMARK_ALL(BM_Insert, IndexConfigList)->ArgList(kSizes); template void BM_SequentialRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); - IndexHandler handler; + ASSERT_OK_AND_ASSIGN(auto handler, IndexHandler::Create()); auto& index = handler.GetIndex(); // Fill in initial elements. @@ -92,7 +92,7 @@ BENCHMARK_ALL(BM_SequentialRead, IndexConfigList)->ArgList(kSizes); template void BM_UniformRandomRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); - IndexHandler handler; + ASSERT_OK_AND_ASSIGN(auto handler, IndexHandler::Create()); auto& index = handler.GetIndex(); // Fill in initial elements. @@ -114,7 +114,7 @@ BENCHMARK_ALL(BM_UniformRandomRead, IndexConfigList)->ArgList(kSizes); template void BM_ExponentialRandomRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); - IndexHandler handler; + ASSERT_OK_AND_ASSIGN(auto handler, IndexHandler::Create()); auto& index = handler.GetIndex(); // Fill in initial elements. @@ -136,7 +136,7 @@ BENCHMARK_ALL(BM_ExponentialRandomRead, IndexConfigList)->ArgList(kSizes); template void BM_Hash(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); - IndexHandler handler; + ASSERT_OK_AND_ASSIGN(auto handler, IndexHandler::Create()); auto& index = handler.GetIndex(); // Fill in initial elements. @@ -152,7 +152,7 @@ void BM_Hash(benchmark::State& state) { ASSERT_OK(index.GetOrAdd(ToKey(i++))); } state.ResumeTiming(); - auto hash = index.GetHash(); + ASSERT_OK_AND_ASSIGN(auto hash, index.GetHash()); benchmark::DoNotOptimize(hash); } } diff --git a/cpp/backend/index/index_handler.h b/cpp/backend/index/index_handler.h index 3f4f1ab88..7e931ebbe 100644 --- a/cpp/backend/index/index_handler.h +++ b/cpp/backend/index/index_handler.h @@ -2,12 +2,14 @@ #include +#include "absl/status/statusor.h" #include "backend/index/cache/cache.h" #include "backend/index/file/index.h" #include "backend/index/index.h" #include "backend/index/leveldb/multi_db/index.h" #include "backend/index/leveldb/single_db/index.h" #include "backend/index/memory/index.h" +#include "backend/structure.h" #include "common/file_util.h" #include "common/type.h" @@ -35,68 +37,49 @@ template class IndexHandler : public IndexHandlerBase { public: - IndexHandler() : index_() {} - Index& GetIndex() { return index_; } - - private: - Index index_; -}; + template + static absl::StatusOr Create(Args&&... args) { + TempDir dir; + Context ctx; + ASSIGN_OR_RETURN(auto index, Index::Open(ctx, dir.GetPath(), + std::forward(args)...)); + return IndexHandler(std::move(ctx), std::move(dir), std::move(index)); + } -// A specialization of the generic IndexHandler for cached index -// implementations. -template -class IndexHandler> - : public IndexHandlerBase { - public: - IndexHandler() : index_(std::move(nested_.GetIndex())) {} - Cached& GetIndex() { return index_; } + Index& GetIndex() { return index_; } private: - IndexHandler nested_; - Cached index_; -}; - -// A specialization of the generic IndexHandler for file-based implementations. -template -class IndexHandler> - : public IndexHandlerBase { - public: - using File = typename FileIndex::File; - IndexHandler() : index_(dir_.GetPath()) {} - - FileIndex& GetIndex() { return index_; } + IndexHandler(Context ctx, TempDir dir, Index idx) + : ctx_(std::move(ctx)), + temp_dir_(std::move(dir)), + index_(std::move(idx)){}; - private: - TempDir dir_; - FileIndex index_; + Context ctx_; + TempDir temp_dir_; + Index index_; }; // A specialization of the generic IndexHandler for leveldb implementation. template class IndexHandler> : public IndexHandlerBase { public: - IndexHandler() - : index_((*SingleLevelDbIndex::Open(dir_.GetPath())) - .template KeySpace('t')) {} + template + static absl::StatusOr Create(Args&&... args) { + TempDir dir; + ASSIGN_OR_RETURN( + auto index, + SingleLevelDbIndex::Open(dir.GetPath(), std::forward(args)...)); + return IndexHandler(std::move(dir), index.template KeySpace('t')); + } + LevelDbKeySpace& GetIndex() { return index_; } private: - TempDir dir_; - LevelDbKeySpace index_; -}; - -// A specialization of the generic IndexHandler for leveldb implementation. -template -class IndexHandler> : public IndexHandlerBase { - public: - IndexHandler() : index_(*MultiLevelDbIndex::Open(dir_.GetPath())) {} - MultiLevelDbIndex& GetIndex() { return index_; } + IndexHandler(TempDir dir, LevelDbKeySpace idx) + : temp_dir_(std::move(dir)), index_(std::move(idx)){}; - private: - TempDir dir_; - MultiLevelDbIndex index_; + TempDir temp_dir_; + LevelDbKeySpace index_; }; - } // namespace } // namespace carmen::backend::index diff --git a/cpp/backend/index/leveldb/BUILD b/cpp/backend/index/leveldb/BUILD index 5f20a5430..6dfc848e6 100644 --- a/cpp/backend/index/leveldb/BUILD +++ b/cpp/backend/index/leveldb/BUILD @@ -22,6 +22,7 @@ cc_binary( "//backend/index/leveldb/single_db:index", "//common:file_util", "//common:type", + "//common:status_test_util", "//third_party/gperftools:profiler", "@com_github_google_benchmark//:benchmark_main", ], diff --git a/cpp/backend/index/leveldb/index_benchmark.cc b/cpp/backend/index/leveldb/index_benchmark.cc index bc604b77d..1afdcfaf9 100644 --- a/cpp/backend/index/leveldb/index_benchmark.cc +++ b/cpp/backend/index/leveldb/index_benchmark.cc @@ -5,6 +5,7 @@ #include "backend/index/leveldb/single_db/index.h" #include "benchmark/benchmark.h" #include "common/file_util.h" +#include "common/status_test_util.h" #include "common/type.h" namespace carmen::backend::index { @@ -16,20 +17,28 @@ namespace { template class SingleIndexBM { public: - explicit SingleIndexBM(std::uint8_t num_indexes) { + static absl::StatusOr Create(std::uint8_t num_indexes) { assert(num_indexes > 0 && "num_indexes must be greater than 0"); + TempDir dir; + ASSIGN_OR_RETURN(auto index, SingleLevelDbIndex::Open(dir.GetPath())); + return SingleIndexBM(num_indexes, index, std::move(dir)); + } + + LevelDbKeySpace& GetIndex(std::uint8_t index) { + return indexes_[index]; + } + + private: + SingleIndexBM(std::uint8_t num_indexes, SingleLevelDbIndex& index, + TempDir dir) + : dir_(std::move(dir)) { // initialize index leveldb index - auto index = *SingleLevelDbIndex::Open(dir_.GetPath()); for (std::uint8_t i = 0; i < num_indexes; ++i) { // create key space indexes_.push_back(index.template KeySpace(i)); } } - LevelDbKeySpace& GetIndex(std::uint8_t index) { - return indexes_[index]; - } - private: TempDir dir_; std::vector> indexes_; }; @@ -37,19 +46,29 @@ class SingleIndexBM { template class MultiIndexBM { public: - explicit MultiIndexBM(std::uint8_t num_indexes) { + static absl::StatusOr Create(std::uint8_t num_indexes) { + using Index = MultiLevelDbIndex; assert(num_indexes > 0 && "num_indexes must be greater than 0"); + std::vector dirs; + std::vector> indexes; for (std::uint8_t i = 0; i < num_indexes; ++i) { auto dir = TempDir(); - indexes_.push_back(*MultiLevelDbIndex::Open(dir.GetPath())); - dirs_.push_back(std::move(dir)); + ASSIGN_OR_RETURN(auto index, Index::Open(dir.GetPath())); + indexes.push_back(std::move(index)); + dirs.push_back(std::move(dir)); } + return MultiIndexBM(std::move(dirs), std::move(indexes)); } + MultiLevelDbIndex& GetIndex(std::uint8_t index) { return indexes_[index]; } private: + MultiIndexBM(std::vector dirs, + std::vector> indexes) + : dirs_(std::move(dirs)), indexes_(std::move(indexes)) {} + std::vector dirs_; std::vector> indexes_; }; @@ -70,21 +89,21 @@ template void BM_Insert(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); auto indexes_count = state.range(1); - LevelDbIndex index(indexes_count); + ASSERT_OK_AND_ASSIGN(auto index, LevelDbIndex::Create(indexes_count)); // Fill in initial elements. for (std::int64_t i = 0; i < pre_loaded_num_elements; i++) { for (std::uint8_t j = 0; j < indexes_count; ++j) { auto& idx = index.GetIndex(j); - *idx.GetOrAdd(ToKey(i)); + ASSERT_OK(idx.GetOrAdd(ToKey(i))); } } auto i = pre_loaded_num_elements; for (auto _ : state) { auto& idx = index.GetIndex(i % indexes_count); - auto id = idx.GetOrAdd(ToKey(i)); - benchmark::DoNotOptimize(*id); + ASSERT_OK_AND_ASSIGN(auto id, idx.GetOrAdd(ToKey(i))); + benchmark::DoNotOptimize(id); ++i; } } @@ -109,21 +128,22 @@ template void BM_SequentialRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); auto indexes_count = state.range(1); - LevelDbIndex index(indexes_count); + ASSERT_OK_AND_ASSIGN(auto index, LevelDbIndex::Create(indexes_count)); // Fill in initial elements. for (std::int64_t i = 0; i < pre_loaded_num_elements; i++) { for (std::uint8_t j = 0; j < indexes_count; ++j) { auto& idx = index.GetIndex(j); - *idx.GetOrAdd(ToKey(i)); + ASSERT_OK(idx.GetOrAdd(ToKey(i))); } } auto i = 0; for (auto _ : state) { auto& idx = index.GetIndex(i % indexes_count); - auto id = idx.GetOrAdd(ToKey(i % pre_loaded_num_elements)); - benchmark::DoNotOptimize(*id); + ASSERT_OK_AND_ASSIGN(auto id, + idx.GetOrAdd(ToKey(i % pre_loaded_num_elements))); + benchmark::DoNotOptimize(id); ++i; } } @@ -148,13 +168,13 @@ template void BM_UniformRandomRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); auto indexes_count = state.range(1); - LevelDbIndex index(indexes_count); + ASSERT_OK_AND_ASSIGN(auto index, LevelDbIndex::Create(indexes_count)); // Fill in initial elements. for (std::int64_t i = 0; i < pre_loaded_num_elements; i++) { for (std::uint8_t j = 0; j < indexes_count; ++j) { auto& idx = index.GetIndex(j); - *idx.GetOrAdd(ToKey(i)); + ASSERT_OK(idx.GetOrAdd(ToKey(i))); } } @@ -164,8 +184,8 @@ void BM_UniformRandomRead(benchmark::State& state) { for (auto _ : state) { auto i = dist(gen); auto& idx = index.GetIndex(i % indexes_count); - auto id = idx.GetOrAdd(ToKey(i)); - benchmark::DoNotOptimize(*id); + ASSERT_OK_AND_ASSIGN(auto id, idx.GetOrAdd(ToKey(i))); + benchmark::DoNotOptimize(id); } } @@ -189,13 +209,13 @@ template void BM_ExponentialRandomRead(benchmark::State& state) { auto pre_loaded_num_elements = state.range(0); auto indexes_count = state.range(1); - LevelDbIndex index(indexes_count); + ASSERT_OK_AND_ASSIGN(auto index, LevelDbIndex::Create(indexes_count)); // Fill in initial elements. for (std::int64_t i = 0; i < pre_loaded_num_elements; i++) { for (std::uint8_t j = 0; j < indexes_count; ++j) { auto& idx = index.GetIndex(j); - *idx.GetOrAdd(ToKey(i)); + ASSERT_OK(idx.GetOrAdd(ToKey(i))); } } @@ -205,8 +225,9 @@ void BM_ExponentialRandomRead(benchmark::State& state) { for (auto _ : state) { auto i = dist(gen); auto& idx = index.GetIndex(int(i) % indexes_count); - auto id = idx.GetOrAdd(ToKey(std::int64_t(i) % pre_loaded_num_elements)); - benchmark::DoNotOptimize(*id); + ASSERT_OK_AND_ASSIGN(auto id, idx.GetOrAdd(ToKey(std::int64_t(i) % + pre_loaded_num_elements))); + benchmark::DoNotOptimize(id); } } diff --git a/cpp/backend/index/leveldb/multi_db/index.h b/cpp/backend/index/leveldb/multi_db/index.h index c58949759..c2cadedd3 100644 --- a/cpp/backend/index/leveldb/multi_db/index.h +++ b/cpp/backend/index/leveldb/multi_db/index.h @@ -40,7 +40,7 @@ class MultiLevelDbIndex : public internal::LevelDbIndexBase { std::array ToDBKey(const K& key) const override { std::array buffer; - memcpy(buffer.data(), &key, sizeof(K)); + std::memcpy(buffer.data(), &key, sizeof(K)); return buffer; }; diff --git a/cpp/backend/index/memory/index_test.cc b/cpp/backend/index/memory/index_test.cc index 002bd28d1..cfb4df18d 100644 --- a/cpp/backend/index/memory/index_test.cc +++ b/cpp/backend/index/memory/index_test.cc @@ -18,16 +18,16 @@ INSTANTIATE_TYPED_TEST_SUITE_P(InMemory, IndexTest, TestIndex); TEST(InMemoryIndexTest, SnapshotShieldsMutations) { TestIndex index; - EXPECT_THAT(index.GetOrAdd(10), IsOkAndHolds(std::pair{0, true})); - EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(std::pair{1, true})); + EXPECT_THAT(index.GetOrAdd(10), IsOkAndHolds(Pair(0, true))); + EXPECT_THAT(index.GetOrAdd(12), IsOkAndHolds(Pair(1, true))); auto snapshot = index.CreateSnapshot(); - EXPECT_THAT(index.GetOrAdd(14), IsOkAndHolds(std::pair{2, true})); + EXPECT_THAT(index.GetOrAdd(14), IsOkAndHolds(Pair(2, true))); TestIndex restored(*snapshot); EXPECT_THAT(restored.Get(10), 0); EXPECT_THAT(restored.Get(12), 1); - EXPECT_THAT(restored.GetOrAdd(14), IsOkAndHolds(std::pair{2, true})); + EXPECT_THAT(restored.GetOrAdd(14), IsOkAndHolds(Pair(2, true))); } TEST(InMemoryIndexTest, SnapshotRecoveryHasSameHash) { @@ -45,7 +45,7 @@ TEST(InMemoryIndexTest, LargeSnapshotRecoveryWorks) { TestIndex index; for (int i = 0; i < kNumElements; i++) { - EXPECT_THAT(index.GetOrAdd(i + 10), IsOkAndHolds(std::pair{i, true})); + EXPECT_THAT(index.GetOrAdd(i + 10), IsOkAndHolds(Pair(i, true))); } ASSERT_OK_AND_ASSIGN(auto hash, index.GetHash()); auto snapshot = index.CreateSnapshot(); diff --git a/cpp/backend/index/memory/linear_hash_index_test.cc b/cpp/backend/index/memory/linear_hash_index_test.cc index b6f47af33..e03d056fd 100644 --- a/cpp/backend/index/memory/linear_hash_index_test.cc +++ b/cpp/backend/index/memory/linear_hash_index_test.cc @@ -8,7 +8,6 @@ namespace carmen::backend::index { namespace { using ::testing::IsOkAndHolds; -using ::testing::Optional; using TestIndex = InMemoryLinearHashIndex; diff --git a/cpp/backend/index/memory/linear_hash_map.h b/cpp/backend/index/memory/linear_hash_map.h index 88bc9c667..64e8d159b 100644 --- a/cpp/backend/index/memory/linear_hash_map.h +++ b/cpp/backend/index/memory/linear_hash_map.h @@ -112,13 +112,13 @@ class LinearHashMap { private: constexpr static const std::uint8_t kInitialHashLength = 2; - // An entry is the type of a single line in the table underlying this map. + // An entry is the type of single line in the table underlying this map. struct Entry { // Entries need to be sorted by their hash within pages. bool operator<(const Entry& other) const { return hash < other.hash; } // The cached hash of this entry. std::size_t hash; - // The key/value pair of this entry. + // The key/value a pair of this entry. entry_type value; }; @@ -128,7 +128,7 @@ class LinearHashMap { // Note: while entries within a single page are sorted, no sorting criteria is // enforced accross pages of a single bucket. struct Page { - // Locates a entry within a list of pages. If found in the current page, a + // Locates an entry within a list of pages. If found in the current page, a // pointer to the corresponding entry is returned. If not found, the // following page in the list is consulted. If there is no such page, a // nullptr is returned. @@ -263,9 +263,9 @@ class LinearHashMap { return {hash, bucket, bucket.Find(hash, key)}; } - // Performs a split of a bucket resulting in the linear grow of the table. In - // each split one bucket is selected and devided into two buckets. While doing - // so, the old bucket is reused and one additional bucket is created. + // Performs a split of a bucket resulting in the linear growth of the table. + // In each split one bucket is selected and divided into two buckets. While + // doing so, the old bucket is reused and one additional bucket is created. // Buckets are split in a round-robbing order. void Split() { assert(next_to_split_ < buckets_.size()); @@ -353,9 +353,9 @@ class LinearHashMap { // Obtains the index of the bucket the given hash key is supposed to be // located. - std::size_t GetBucket(std::size_t hashkey) const { - std::size_t bucket = hashkey & high_mask_; - return bucket >= buckets_.size() ? hashkey & low_mask_ : bucket; + std::size_t GetBucket(std::size_t hash_key) const { + std::size_t bucket = hash_key & high_mask_; + return bucket >= buckets_.size() ? hash_key & low_mask_ : bucket; } // A hasher to compute hashes for keys. diff --git a/cpp/backend/index/test_util.h b/cpp/backend/index/test_util.h index 0a88ce613..7e1f18b5e 100644 --- a/cpp/backend/index/test_util.h +++ b/cpp/backend/index/test_util.h @@ -31,13 +31,13 @@ class IndexTest : public testing::Test {}; TYPED_TEST_SUITE_P(IndexTest); TYPED_TEST_P(IndexTest, TypeProperties) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_TRUE(std::is_move_constructible_v); } TYPED_TEST_P(IndexTest, IdentifiersAreAssignedInorder) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(std::pair(0, true))); EXPECT_THAT(index.GetOrAdd(2), IsOkAndHolds(std::pair(1, true))); @@ -45,7 +45,7 @@ TYPED_TEST_P(IndexTest, IdentifiersAreAssignedInorder) { } TYPED_TEST_P(IndexTest, SameKeyLeadsToSameIdentifier) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.GetOrAdd(1), IsOkAndHolds(std::pair(0, true))); EXPECT_THAT(index.GetOrAdd(2), IsOkAndHolds(std::pair(1, true))); @@ -54,7 +54,7 @@ TYPED_TEST_P(IndexTest, SameKeyLeadsToSameIdentifier) { } TYPED_TEST_P(IndexTest, ContainsIdentifiesIndexedElements) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.Get(1), StatusIs(absl::StatusCode::kNotFound, _)); @@ -73,7 +73,7 @@ TYPED_TEST_P(IndexTest, ContainsIdentifiesIndexedElements) { } TYPED_TEST_P(IndexTest, GetRetrievesPresentKeys) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.Get(1), StatusIs(absl::StatusCode::kNotFound, _)); EXPECT_THAT(index.Get(2), StatusIs(absl::StatusCode::kNotFound, _)); @@ -89,14 +89,14 @@ TYPED_TEST_P(IndexTest, GetRetrievesPresentKeys) { } TYPED_TEST_P(IndexTest, EmptyIndexHasHashEqualsZero) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.GetHash(), IsOkAndHolds(Hash{})); } TYPED_TEST_P(IndexTest, IndexHashIsEqualToInsertionOrder) { Hash hash; - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); EXPECT_THAT(index.GetHash(), IsOkAndHolds(hash)); ASSERT_OK(index.GetOrAdd(12)); @@ -111,14 +111,14 @@ TYPED_TEST_P(IndexTest, IndexHashIsEqualToInsertionOrder) { } TYPED_TEST_P(IndexTest, CanProduceMemoryFootprint) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); auto summary = index.GetMemoryFootprint(); EXPECT_GT(summary.GetTotal(), Memory(0)); } TYPED_TEST_P(IndexTest, HashesMatchReferenceImplementation) { - IndexHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, IndexHandler::Create()); auto& index = wrapper.GetIndex(); auto& reference_index = wrapper.GetReferenceIndex(); @@ -157,8 +157,8 @@ class MockIndex { MOCK_METHOD(MemoryFootprint, GetMemoryFootprint, (), (const)); }; -// A movable wrapper of a mock index. This may be required when a index needs to -// be moved into position. +// A movable wrapper of a mock index. This may be required when an index needs +// to be moved into position. template class MockIndexWrapper { public: @@ -171,7 +171,7 @@ class MockIndexWrapper { } MockIndexWrapper() : index_(std::make_unique>()) {} - MockIndexWrapper(MockIndexWrapper&&) = default; + MockIndexWrapper(MockIndexWrapper&&) noexcept = default; absl::StatusOr> GetOrAdd(const K& key) { return index_->GetOrAdd(key); diff --git a/cpp/backend/multimap/memory/multimap.h b/cpp/backend/multimap/memory/multimap.h index 2db6711be..78fb43477 100644 --- a/cpp/backend/multimap/memory/multimap.h +++ b/cpp/backend/multimap/memory/multimap.h @@ -101,10 +101,7 @@ class InMemoryMultiMap { // Writes all data to the underlying file. absl::Status Flush() { // Start by creating the directory. - if (!CreateDirectory(file_.parent_path())) { - return absl::InternalError(absl::StrFormat( - "Unable to create parent directory: %s", file_.parent_path())); - } + RETURN_IF_ERROR(CreateDirectory(file_.parent_path())); std::fstream out(file_, std::ios::binary | std::ios::out); auto check_stream = [&]() -> absl::Status { diff --git a/cpp/backend/store/BUILD b/cpp/backend/store/BUILD index 3543d1d94..76e346d58 100644 --- a/cpp/backend/store/BUILD +++ b/cpp/backend/store/BUILD @@ -19,6 +19,7 @@ cc_library( "//backend/store/memory:store", "//common:file_util", "//common:type", + "//common:status_util", ], ) diff --git a/cpp/backend/store/file/store.h b/cpp/backend/store/file/store.h index e742c8af2..55139a2ba 100644 --- a/cpp/backend/store/file/store.h +++ b/cpp/backend/store/file/store.h @@ -24,7 +24,7 @@ namespace internal { // The FileStoreBase is the common bases of file-backed implementations of a // mutable key/value store. It provides mutation, lookup, and global state -// hashing support. Hashing can occure eager (before evicting pages) or lazy, +// hashing support. Hashing can occur eager (before evicting pages) or lazy, // when requesting hash computations. template class F, std::size_t page_size = 32, bool eager_hashing = true> @@ -68,25 +68,14 @@ class FileStoreBase { using value_type = V; // The page size in byte used by this store as configured. This may be less - // then the actual page size, which may be larger due to alignment and padding + // than the actual page size, which may be larger due to alignment and padding // constraints. constexpr static std::size_t kPageSize = page_size; // A factory function creating an instance of this store type. static absl::StatusOr Open( Context&, const std::filesystem::path& directory, - std::size_t hash_branching_factor = 32) { - // Make sure the directory exists. - if (!CreateDirectory(directory)) { - return absl::InternalError( - absl::StrFormat("Unable to create parent directory %s", directory)); - } - auto store = FileStoreBase(directory, hash_branching_factor); - if (std::filesystem::exists(store.hash_file_)) { - RETURN_IF_ERROR(store.hashes_->LoadFromFile(store.hash_file_)); - } - return store; - } + std::size_t hash_branching_factor = 32); // Supports instances to be moved. FileStoreBase(FileStoreBase&&) = default; @@ -155,19 +144,25 @@ class FileStoreBase { PageProvider(PagePool& pool) : pool_(pool) {} std::span GetPageData(PageId id) override { - return std::as_bytes(std::span(pool_.template Get(id).AsArray())); + constexpr std::span const kEmpty; + auto page = pool_.template Get(id); + if (!page.ok()) { + return kEmpty; + } + return std::as_bytes(std::span(page->AsReference().AsArray())); } private: PagePool& pool_; }; - // The number of elements per page, used for page and offset computaiton. + // The number of elements per page, used for page and offset computation. constexpr static std::size_t kNumElementsPerPage = Page::kNumElementsPerPage; // Creates a new file store maintaining its content in the given directory and // using the provided branching factor for its hash computation. - FileStoreBase(std::filesystem::path directory, + FileStoreBase(std::unique_ptr> file, + std::filesystem::path hash_file, std::size_t hash_branching_factor); // The page pool handling the in-memory buffer of pages fetched from disk. The @@ -175,7 +170,7 @@ class FileStoreBase { // store is moved. mutable std::unique_ptr pool_; - // The data structure hanaging the hashing of states. The hashes are placed in + // The data structure managing the hashing of states. The hashes are placed in // a unique pointer to ensure pointer stability when the store is moved. mutable std::unique_ptr hashes_; @@ -183,16 +178,35 @@ class FileStoreBase { std::filesystem::path hash_file_; }; +template class F, + std::size_t page_size, bool eager_hashing> +requires File)>> + absl::StatusOr> + FileStoreBase::Open( + Context&, const std::filesystem::path& directory, + std::size_t hash_branching_factor) { + // Make sure the directory exists. + RETURN_IF_ERROR(CreateDirectory(directory)); + ASSIGN_OR_RETURN(auto file, F::Open(directory / "data.dat")); + auto store = + FileStoreBase(std::make_unique>(std::move(file)), + directory / "hash.dat", hash_branching_factor); + if (std::filesystem::exists(store.hash_file_)) { + RETURN_IF_ERROR(store.hashes_->LoadFromFile(store.hash_file_)); + } + return store; +} + template class F, std::size_t page_size, bool eager_hashing> requires File)>> FileStoreBase::FileStoreBase( - std::filesystem::path directory, std::size_t hash_branching_factor) - : pool_(std::make_unique( - std::make_unique>(directory / "data.dat"))), + std::unique_ptr> file, std::filesystem::path hash_file, + std::size_t hash_branching_factor) + : pool_(std::make_unique(std::move(file))), hashes_(std::make_unique(std::make_unique(*pool_), hash_branching_factor)), - hash_file_(directory / "hash.dat") { + hash_file_(std::move(hash_file)) { pool_->AddListener(std::make_unique(*hashes_)); } @@ -200,8 +214,9 @@ template class F, std::size_t page_size, bool eager_hashing> requires File)>> absl::Status FileStoreBase::Set(const K& key, V value) { - auto& trg = pool_->template Get( - key / kNumElementsPerPage)[key % kNumElementsPerPage]; + ASSIGN_OR_RETURN(Page & page, + pool_->template Get(key / kNumElementsPerPage)); + auto& trg = page[key % kNumElementsPerPage]; if (trg != value) { trg = value; pool_->MarkAsDirty(key / kNumElementsPerPage); @@ -216,8 +231,9 @@ requires File)>> StatusOrRef FileStoreBase::Get( const K& key) const { - return pool_->template Get( - key / kNumElementsPerPage)[key % kNumElementsPerPage]; + ASSIGN_OR_RETURN(Page & page, + pool_->template Get(key / kNumElementsPerPage)); + return page[key % kNumElementsPerPage]; } template class F, @@ -231,7 +247,9 @@ template class F, std::size_t page_size, bool eager_hashing> requires File)>> absl::Status FileStoreBase::Flush() { - if (pool_) pool_->Flush(); + if (pool_) { + RETURN_IF_ERROR(pool_->Flush()); + } if (hashes_) { RETURN_IF_ERROR(hashes_->SaveToFile(hash_file_)); } @@ -243,7 +261,9 @@ template class F, requires File)>> absl::Status FileStoreBase::Close() { RETURN_IF_ERROR(Flush()); - if (pool_) pool_->Close(); + if (pool_) { + RETURN_IF_ERROR(pool_->Close()); + } return absl::OkStatus(); } diff --git a/cpp/backend/store/hash_tree.cc b/cpp/backend/store/hash_tree.cc index 1c9b1d797..494a5845f 100644 --- a/cpp/backend/store/hash_tree.cc +++ b/cpp/backend/store/hash_tree.cc @@ -146,13 +146,11 @@ absl::Status HashTree::SaveToFile(const std::filesystem::path& file) { absl::StrFormat("Could not open file %s for writing.", file)); } - auto write_scalar = [&out, &file](auto& data) { + auto write_scalar = [&](auto& data) -> absl::Status { out.write(reinterpret_cast(&data), sizeof(data)); - if (!out.good()) { - return absl::InternalError( - absl::StrFormat("Could not write to file %s.", file)); - } - return absl::OkStatus(); + if (out.good()) return absl::OkStatus(); + return absl::InternalError( + absl::StrFormat("Could not write to file %s.", file)); }; RETURN_IF_ERROR(write_scalar(branching_factor)); @@ -199,13 +197,11 @@ absl::Status HashTree::LoadFromFile(const std::filesystem::path& file) { "File %s is too short. Needed 40, got %d bytes.", file, size)); } - auto read_scalar = [&in, &file](auto& data) { + auto read_scalar = [&](auto& data) -> absl::Status { in.read(reinterpret_cast(&data), sizeof(data)); - if (!in.good()) { - return absl::InternalError( - absl::StrFormat("Could not read from file %s.", file)); - } - return absl::OkStatus(); + if (in.good()) return absl::OkStatus(); + return absl::InternalError( + absl::StrFormat("Could not read from file %s.", file)); }; in.seekg(0, std::ios::beg); diff --git a/cpp/backend/store/hash_tree.h b/cpp/backend/store/hash_tree.h index 8732b4bdc..8b928ac22 100644 --- a/cpp/backend/store/hash_tree.h +++ b/cpp/backend/store/hash_tree.h @@ -25,7 +25,7 @@ class PageSource { // A HashTree is managing the hashes of a list of pages as well as the // aggregation thereof to a single global hash. // -// This class maintains a hirarchy of partially aggregated page hashes, +// This class maintains a hierarchy of partially aggregated page hashes, // as well as dirty state information. Whenever a full hash is requested, dirty // (=outdated) hashes are refreshed, before a new global hash is obtained. class HashTree { @@ -59,12 +59,12 @@ class HashTree { void MarkDirty(PageId page); // Computes a global hash for all pages managed by this HashTree. It will - // update outdated partical hashes cached internally, which may imply the need + // update outdated partial hashes cached internally, which may imply the need // for fetching dirty pages. Hash GetHash(); // Saves the hashes of this tree into the given file. Before saving them, all - // outdated hashes are implicitely refreshed. + // outdated hashes are implicitly refreshed. absl::Status SaveToFile(const std::filesystem::path& file); // Discards the current content of this HashTree and loads all hashes from the @@ -74,11 +74,11 @@ class HashTree { absl::Status LoadFromFile(const std::filesystem::path& file); // Saves the hashes of this tree into the given leveldb path. Before saving - // them, all outdated hashes are implicitely refreshed. + // them, all outdated hashes are implicitly refreshed. absl::Status SaveToLevelDb(const std::filesystem::path& path); // Saves the hashes of this tree into the given leveldb instance. Before - // saving them, all outdated hashes are implicitely refreshed. + // saving them, all outdated hashes are implicitly refreshed. absl::Status SaveToLevelDb(LevelDb& leveldb); // Discards the current content of this HashTree and loads all hashes from the diff --git a/cpp/backend/store/memory/BUILD b/cpp/backend/store/memory/BUILD index 628b7c241..92e8d60d2 100644 --- a/cpp/backend/store/memory/BUILD +++ b/cpp/backend/store/memory/BUILD @@ -19,6 +19,7 @@ cc_test( deps = [ ":store", "//common:status_test_util", + "//common:file_util", "@com_google_googletest//:gtest_main", ], ) diff --git a/cpp/backend/store/memory/store.h b/cpp/backend/store/memory/store.h index ec749c84c..1332e5572 100644 --- a/cpp/backend/store/memory/store.h +++ b/cpp/backend/store/memory/store.h @@ -36,9 +36,10 @@ class InMemoryStore { constexpr static std::size_t kPageSize = page_size; // A factory function creating an instance of this store type. - static absl::StatusOr Open(Context&, - const std::filesystem::path&) { - return InMemoryStore(); + static absl::StatusOr Open( + Context&, const std::filesystem::path&, + std::size_t hash_branching_factor = 32) { + return InMemoryStore(hash_branching_factor); } // Creates a new InMemoryStore using the provided value as the @@ -177,7 +178,7 @@ class InMemoryStore { // wrapped in a unique pointer to facilitate pointer stability under move. std::unique_ptr pages_; - // The data structure hanaging the hashing of states. + // The data structure managing the hashing of states. mutable HashTree hashes_; }; diff --git a/cpp/backend/store/memory/store_test.cc b/cpp/backend/store/memory/store_test.cc index 3e9985df9..dfc495954 100644 --- a/cpp/backend/store/memory/store_test.cc +++ b/cpp/backend/store/memory/store_test.cc @@ -2,6 +2,7 @@ #include +#include "common/file_util.h" #include "common/status_test_util.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -22,7 +23,9 @@ TEST(InMemoryStoreTest, TypeTraits) { } TEST(InMemoryStoreTest, SnapshotShieldsMutations) { - Store store; + TempDir dir; + Context ctx; + ASSERT_OK_AND_ASSIGN(auto store, Store::Open(ctx, dir.GetPath())); ASSERT_OK(store.Set(10, 12)); EXPECT_THAT(store.Get(10), IsOkAndHolds(12)); @@ -38,7 +41,10 @@ TEST(InMemoryStoreTest, SnapshotShieldsMutations) { } TEST(InMemoryStoreTest, SnapshotRecoveryHasSameHash) { - Store store; + TempDir dir; + Context ctx; + ASSERT_OK_AND_ASSIGN(auto store, Store::Open(ctx, dir.GetPath())); + ASSERT_OK(store.Set(10, 12)); ASSERT_OK_AND_ASSIGN(auto hash, store.GetHash()); auto snapshot = store.CreateSnapshot(); @@ -49,8 +55,10 @@ TEST(InMemoryStoreTest, SnapshotRecoveryHasSameHash) { TEST(InMemoryStoreTest, LargeSnapshotRecoveryWorks) { constexpr const int kNumElements = 100000; + TempDir dir; + Context ctx; + ASSERT_OK_AND_ASSIGN(auto store, Store::Open(ctx, dir.GetPath())); - Store store; for (int i = 0; i < kNumElements; i++) { ASSERT_OK(store.Set(i, i + 10)); } diff --git a/cpp/backend/store/store.h b/cpp/backend/store/store.h index 7dae2f957..f42fa8cf2 100644 --- a/cpp/backend/store/store.h +++ b/cpp/backend/store/store.h @@ -12,7 +12,7 @@ namespace carmen::backend::store { // A snapshot of the state of a store providing access to the contained data -// frozen at it creation time. This definies an interface for store +// frozen at it creation time. This defines an interface for store // implementation specific implementations. // // The life cycle of a snapshot defines the duration of its availability. diff --git a/cpp/backend/store/store_benchmark.cc b/cpp/backend/store/store_benchmark.cc index 1f1febde7..14c85d577 100644 --- a/cpp/backend/store/store_benchmark.cc +++ b/cpp/backend/store/store_benchmark.cc @@ -38,9 +38,10 @@ void InitStore(Store& store, std::size_t num_elements) { // Benchmarks the sequential insertion of keys into stores. template void BM_SequentialInsert(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); for (auto _ : state) { - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); auto& store = wrapper.GetStore(); for (int i = 0; i < num_elements; i++) { ASSERT_OK(store.Set(i, Value{})); @@ -53,11 +54,12 @@ BENCHMARK_ALL(BM_SequentialInsert, StoreConfigList)->ArgList(kSizes); // Benchmarks the appending of new elements to the store. template void BM_Insert(benchmark::State& state) { + using Handler = StoreHandler; // The size of the store before the inserts. auto num_elements = state.range(0); // Initialize the store with the initial number of elements. - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); auto& store = wrapper.GetStore(); InitStore(store, num_elements); @@ -73,8 +75,9 @@ BENCHMARK_ALL(BM_Insert, StoreConfigList)->ArgList(kSizes); // Benchmarks sequential read of keys. template void BM_SequentialRead(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -89,11 +92,12 @@ void BM_SequentialRead(benchmark::State& state) { BENCHMARK_ALL(BM_SequentialRead, StoreConfigList)->ArgList(kSizes); -// Benchmarks random, uniformely distributed reads +// Benchmarks random, uniformly distributed reads template void BM_UniformRandomRead(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -113,8 +117,9 @@ BENCHMARK_ALL(BM_UniformRandomRead, StoreConfigList)->ArgList(kSizes); // Benchmarks random, exponentially distributed reads template void BM_ExponentialRandomRead(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -134,8 +139,9 @@ BENCHMARK_ALL(BM_ExponentialRandomRead, StoreConfigList)->ArgList(kSizes); // Benchmarks sequential writes of keys. template void BM_SequentialWrite(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -150,11 +156,12 @@ void BM_SequentialWrite(benchmark::State& state) { BENCHMARK_ALL(BM_SequentialWrite, StoreConfigList)->ArgList(kSizes); -// Benchmarks random, uniformely distributed writes. +// Benchmarks random, uniformly distributed writes. template void BM_UniformRandomWrite(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -175,8 +182,9 @@ BENCHMARK_ALL(BM_UniformRandomWrite, StoreConfigList)->ArgList(kSizes); // Benchmarks sequential read of keys. template void BM_ExponentialRandomWrite(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -196,8 +204,9 @@ BENCHMARK_ALL(BM_ExponentialRandomWrite, StoreConfigList)->ArgList(kSizes); template void RunHashSequentialUpdates(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -231,8 +240,9 @@ BENCHMARK_ALL(BM_HashSequentialUpdates, StoreConfigList)->ArgList(kSizes); template void RunHashUniformUpdates(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); @@ -270,8 +280,9 @@ BENCHMARK_ALL(BM_HashUniformUpdates, StoreConfigList)->ArgList(kSizes); template void RunHashExponentialUpdates(benchmark::State& state) { + using Handler = StoreHandler; auto num_elements = state.range(0); - StoreHandler wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, Handler::Create()); // Initialize the store with the total number of elements. auto& store = wrapper.GetStore(); diff --git a/cpp/backend/store/store_handler.h b/cpp/backend/store/store_handler.h index 1c4a76a23..b4361c7f9 100644 --- a/cpp/backend/store/store_handler.h +++ b/cpp/backend/store/store_handler.h @@ -7,6 +7,7 @@ #include "backend/store/file/store.h" #include "backend/store/memory/store.h" #include "common/file_util.h" +#include "common/status_util.h" namespace carmen::backend::store { namespace { @@ -15,28 +16,6 @@ namespace { template using ReferenceStore = InMemoryStore; -// A base type for StoreHandler types (see below) exposing common definitions. -template -class StoreHandlerBase { - public: - constexpr static std::size_t kPageSize = page_size; - constexpr static std::size_t kBranchingFactor = branching_factor; - - StoreHandlerBase() : reference_(branching_factor) {} - - // Obtains access to a reference store implementation to be used to compare - // the handled store with. The reference type is configured to use the same - // page size and branching factor. - auto& GetReferenceStore() { return reference_; } - - // A temporary directory files of the maintained store a placed in. - std::filesystem::path GetStoreDirectory() const { return dir_; } - - private: - ReferenceStore reference_; - TempDir dir_; -}; - // A generic store handler enclosing the setup and tear down of various store // implementations for the generic unit tests in store_test.cc and benchmarks in // store_benchmark.cc. A handler holds an instance of a store configured with a @@ -46,32 +25,36 @@ class StoreHandlerBase { // This generic StoreHandler is a mere wrapper on a store reference, while // specializations may add additional setup and tear-down operations. template -class StoreHandler - : public StoreHandlerBase { +class StoreHandler { public: - using StoreHandlerBase::GetStoreDirectory; - StoreHandler() - : store_(*Store::Open(context_, GetStoreDirectory(), branching_factor)) {} - Store& GetStore() { return store_; } + constexpr static std::size_t kPageSize = Store::kPageSize; + constexpr static std::size_t kBranchingFactor = branching_factor; - private: - Context context_; - Store store_; -}; + template + static absl::StatusOr Create(Args&&... args) { + TempDir dir; + Context ctx; + ASSIGN_OR_RETURN(auto store, + Store::Open(ctx, dir.GetPath(), branching_factor, + std::forward(args)...)); + return StoreHandler(std::move(store), std::move(ctx), std::move(dir)); + } -// A specialization of a StoreHandler for InMemoryStores handling ingoring the -// creation/deletion of temporary files and directories. -template -class StoreHandler, branching_factor> - : public StoreHandlerBase { - public: - StoreHandler() : store_(branching_factor) {} + Store& GetStore() { return store_; } - InMemoryStore& GetStore() { return store_; } + auto& GetReferenceStore() { return reference_; } private: - InMemoryStore store_; + StoreHandler(Store store, Context context, TempDir dir) + : dir_(std::move(dir)), + context_(std::move(context)), + store_(std::move(store)), + reference_(branching_factor) {} + + TempDir dir_; + Context context_; + Store store_; + ReferenceStore reference_; }; } // namespace } // namespace carmen::backend::store diff --git a/cpp/backend/store/store_test.cc b/cpp/backend/store/store_test.cc index 6ab846f3f..ac1d9f930 100644 --- a/cpp/backend/store/store_test.cc +++ b/cpp/backend/store/store_test.cc @@ -35,13 +35,13 @@ class StoreTest : public testing::Test {}; TYPED_TEST_SUITE_P(StoreTest); TYPED_TEST_P(StoreTest, TypeProperties) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); EXPECT_TRUE(Store>); EXPECT_TRUE(std::is_move_constructible_v); } TYPED_TEST_P(StoreTest, UninitializedValuesAreZero) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); EXPECT_THAT(store.Get(0), IsOkAndHolds(Value{})); EXPECT_THAT(store.Get(10), IsOkAndHolds(Value{})); @@ -49,7 +49,7 @@ TYPED_TEST_P(StoreTest, UninitializedValuesAreZero) { } TYPED_TEST_P(StoreTest, DataCanBeAddedAndRetrieved) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); EXPECT_THAT(store.Get(10), IsOkAndHolds(Value{})); EXPECT_THAT(store.Get(12), IsOkAndHolds(Value{})); @@ -64,7 +64,7 @@ TYPED_TEST_P(StoreTest, DataCanBeAddedAndRetrieved) { } TYPED_TEST_P(StoreTest, EntriesCanBeUpdated) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); EXPECT_THAT(store.Get(10), IsOkAndHolds(Value{})); ASSERT_OK(store.Set(10, Value{12})); @@ -74,13 +74,13 @@ TYPED_TEST_P(StoreTest, EntriesCanBeUpdated) { } TYPED_TEST_P(StoreTest, EmptyStoreHasZeroHash) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); EXPECT_THAT(store.GetHash(), IsOkAndHolds(Hash{})); } TYPED_TEST_P(StoreTest, HashesChangeWithUpdates) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); ASSERT_OK_AND_ASSIGN(auto empty_hash, store.GetHash()); @@ -94,7 +94,7 @@ TYPED_TEST_P(StoreTest, HashesChangeWithUpdates) { } TYPED_TEST_P(StoreTest, HashesCoverMultiplePages) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); ASSERT_OK_AND_ASSIGN(auto empty_hash, store.GetHash()); @@ -111,7 +111,7 @@ TYPED_TEST_P(StoreTest, HashesCoverMultiplePages) { TYPED_TEST_P(StoreTest, KnownHashesAreReproduced) { // We only hard-code hashes for a subset of the configurations. - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); EXPECT_THAT(store.GetHash(), IsOkAndHolds(Hash{})); @@ -187,7 +187,7 @@ TYPED_TEST_P(StoreTest, HashesRespectBranchingFactor) { // factor empty pages. static_assert(TypeParam::kPageSize % sizeof(Value) == 0); constexpr auto kElementsPerPage = TypeParam::kPageSize / sizeof(Value); - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); // Initialize branching_factor * 2 pages. @@ -224,7 +224,7 @@ TYPED_TEST_P(StoreTest, HashesRespectBranchingFactor) { TYPED_TEST_P(StoreTest, HashesEqualReferenceImplementation) { constexpr int N = 100; - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); auto& reference = wrapper.GetReferenceStore(); @@ -244,11 +244,11 @@ TYPED_TEST_P(StoreTest, HashesEqualReferenceImplementation) { } TYPED_TEST_P(StoreTest, HashesRespectEmptyPages) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); auto& reference = wrapper.GetReferenceStore(); - // Implictitly create empty pages by asking for an element with a high ID. + // Implicitly create empty pages by asking for an element with a high ID. ASSERT_OK(reference.Get(10000)); ASSERT_OK(store.Get(10000)); @@ -260,7 +260,7 @@ TYPED_TEST_P(StoreTest, HashesRespectEmptyPages) { } TYPED_TEST_P(StoreTest, CanProduceMemoryFootprint) { - TypeParam wrapper; + ASSERT_OK_AND_ASSIGN(auto wrapper, TypeParam::Create()); auto& store = wrapper.GetStore(); auto summary = store.GetMemoryFootprint(); EXPECT_GT(summary.GetTotal(), Memory(0)); diff --git a/cpp/backend/structure.h b/cpp/backend/structure.h index 0356f2580..6656c6e63 100644 --- a/cpp/backend/structure.h +++ b/cpp/backend/structure.h @@ -57,12 +57,12 @@ concept Structure = requires(S a) { // Structures must be closeable. { a.Close() } -> std::same_as; } -// All structures must be moveable. +// All structures must be movable. &&std::is_move_constructible_v // Structures must provide memory-footprint information. && MemoryFootprintProvider; -// Extends the requiremetns of a data structure by an additional need for +// Extends the requirements of a data structure by an additional need for // supporting effective full-state hashing. template concept HashableStructure = Structure && requires(S a) { diff --git a/cpp/common/BUILD b/cpp/common/BUILD index fda0dee30..5b8c8be26 100644 --- a/cpp/common/BUILD +++ b/cpp/common/BUILD @@ -52,7 +52,6 @@ cc_test( cc_library( name = "type", - srcs = ["type.cc"], hdrs = ["type.h"], visibility = ["//visibility:public"], deps = [ diff --git a/cpp/common/account_state.cc b/cpp/common/account_state.cc index ed7a2327a..3cf24c903 100644 --- a/cpp/common/account_state.cc +++ b/cpp/common/account_state.cc @@ -1,6 +1,5 @@ #include "common/account_state.h" -#include #include namespace carmen { diff --git a/cpp/common/benchmark_benchmark.cc b/cpp/common/benchmark_benchmark.cc index 43b69ae02..274dbe8f8 100644 --- a/cpp/common/benchmark_benchmark.cc +++ b/cpp/common/benchmark_benchmark.cc @@ -1,6 +1,5 @@ #include "common/benchmark.h" -#include #include #include #include @@ -8,13 +7,11 @@ #include "benchmark/benchmark.h" -using pair_int_double = std::pair; - // Define a list of types to run generic benchmarks on. BENCHMARK_TYPE_LIST(MyList, int, float, std::string); // Define a second list of difficult cases. -// Types with a , (comma) in the name need to be put in parantheses. +// Types with a , (comma) in the name need to be put in parentheses. BENCHMARK_TYPE_LIST(DifficultCases, std::vector, (std::pair)); template diff --git a/cpp/common/fstream.cc b/cpp/common/fstream.cc index f706cb74b..be07d90aa 100644 --- a/cpp/common/fstream.cc +++ b/cpp/common/fstream.cc @@ -61,4 +61,4 @@ absl::Status FStream::Close() { bool FStream::IsOpen() const { return fs_.is_open(); } -} // namespace carmen \ No newline at end of file +} // namespace carmen diff --git a/cpp/common/fstream.h b/cpp/common/fstream.h index d30511635..55826e40a 100644 --- a/cpp/common/fstream.h +++ b/cpp/common/fstream.h @@ -24,6 +24,11 @@ class FStream { template absl::Status Read(std::span buffer); + // Reads a value of given type from the file. + // Returns an error if read failed. + template + absl::Status Read(T& buffer); + // Reads the number of elements from file specified by size of the buffer. // When the end of the file is reached, the eof flag is swallowed. Returns // number of elements read. Returns an error if read failed. @@ -35,9 +40,14 @@ class FStream { template absl::Status Write(std::span data); + // Writes value of given type into the file. + // Returns an error if write failed. + template + absl::Status Write(const T& data); + // Seek to the given offset in the file. Should be used when reading from file // at certain position. Returns an error if seekg failed. - absl::Status Seekg(std::size_t offset, std::ios::seekdir dir); + absl::Status Seekg(std::size_t offset, std::ios::seekdir dir = std::ios::beg); // Get the current position in the file. Should be used when reading from // file. Returns an error if tellg failed. @@ -45,7 +55,7 @@ class FStream { // Seek to the given offset in the file. Should be used when writing to file // at certain position. Returns an error if seekp failed. - absl::Status Seekp(std::size_t offset, std::ios::seekdir dir); + absl::Status Seekp(std::size_t offset, std::ios::seekdir dir = std::ios::beg); // Get the current position in the file. Should be used when writing to file. // Returns an error if tellp failed. @@ -76,6 +86,11 @@ absl::Status FStream::Read(std::span buffer) { absl::StrFormat("Failed to read from file %s.", path_.string())); } +template +absl::Status FStream::Read(T& buffer) { + return Read(std::span(&buffer, 1)); +} + template absl::StatusOr FStream::ReadUntilEof(std::span buffer) { // Reading from closed file returns same flags as reading until eof, so we @@ -102,4 +117,9 @@ absl::Status FStream::Write(std::span data) { return absl::InternalError( absl::StrFormat("Failed to write into file %s.", path_.string())); } + +template +absl::Status FStream::Write(const T& data) { + return Write(std::span(&data, 1)); +} } // namespace carmen diff --git a/cpp/common/hash.h b/cpp/common/hash.h index d5f8f3046..93e4c3237 100644 --- a/cpp/common/hash.h +++ b/cpp/common/hash.h @@ -19,7 +19,7 @@ class Sha256Impl; // functions, and consume the final hash using GetHash(). Once a hash is // consumed, no more input may be added. // -// Instances can be reused for multiple hash computation by reseting them +// Instances can be reused for multiple hash computation by resetting them // between hashing operations. This is more efficient than recreating a new // instance for each step. class Sha256Hasher { @@ -44,15 +44,15 @@ class Sha256Hasher { // A no-op serving as the base case for ingesting lists of trivial types. void Ingest() {} - // A convenience variant of the fuction above, supporting the hashing of + // A convenience variant of the function above, supporting the hashing of // all trivial types. template void Ingest(const T& value) { Ingest(reinterpret_cast(&value), sizeof(T)); } - // An extension of the fuction above, supporting the ingestion of a list - // of trival objects. + // An extension of the function above, supporting the ingestion of a list + // of trivial objects. template void Ingest(const First& first, const Rest&... rest) { Ingest(first); @@ -67,14 +67,14 @@ class Sha256Hasher { void Reset(); private: - // The actual implementation of the hasher is hidden behind a interanl + // The actual implementation of the hasher is hidden behind an internal // data type (Pimpl-pattern) to avoid including headers referencing // implementation details into this header file, and those avoiding their // import in other files. std::unique_ptr _impl; }; -// A utility fuiction to hash a list of trivial elements using the given hasher +// A utility function to hash a list of trivial elements using the given hasher // instance. The state of the handed in hasher is reset before ingesting the // provided list of elements. template @@ -85,7 +85,7 @@ Hash GetHash(Sha256Hasher& hasher, const Elements&... elements) { } // A utility function to compute the SHA256 hash of a list of trivial elements. -// It iternally creats a Sha256Hasher instance for computing the hash. If +// It internally creates a Sha256Hasher instance for computing the hash. If // multiple hashes are to be computed, consider creating such an instance in the // caller scope and reusing the instance for all invocations. template diff --git a/cpp/common/heterogenous_map.h b/cpp/common/heterogenous_map.h index fdaddfe2b..f8b158659 100644 --- a/cpp/common/heterogenous_map.h +++ b/cpp/common/heterogenous_map.h @@ -10,7 +10,7 @@ namespace carmen { // A HeterogenousMap is a map retaining values of various types, indexed by // their types. Thus, for each type T a value of type T may be maintained, which -// can be retrieved and modified. It is mainly intended for for enviroments +// can be retrieved and modified. It is mainly intended for environments // depending on generic extensions following the open-closed principle. class HeterogenousMap { public: @@ -62,7 +62,7 @@ class HeterogenousMap { } private: - // A polymorthic base type for all entries. Its main purpose is to provide a + // A polymorphic base type for all entries. Its main purpose is to provide a // common base type for all stored elements and a virtual destructor for those // facilitating proper cleanup on destruction. struct EntryBase { @@ -83,7 +83,7 @@ class HeterogenousMap { }; // The underlying data structure mapping types to entries. Entries are - // referenced through their polymorthic base type. + // referenced through their polymorphic base type. absl::flat_hash_map> map_; }; diff --git a/cpp/common/memory_usage.h b/cpp/common/memory_usage.h index 551c1b29f..16f98eff1 100644 --- a/cpp/common/memory_usage.h +++ b/cpp/common/memory_usage.h @@ -99,8 +99,8 @@ constexpr static const Memory PiB = 1024 * TiB; constexpr static const Memory EiB = 1024 * PiB; // A MemoryFootprint describes the memory usage of a DAG shaped object graph. -// Each node is the root of a DAG of objects, where each node is a object -// desribed by a MemoryFootprint instance including its memory usage, and each +// Each node is the root of a DAG of objects, where each node is an object +// described by a MemoryFootprint instance including its memory usage, and each // edge is labeled by a field name. class MemoryFootprint { public: diff --git a/cpp/common/status_util.h b/cpp/common/status_util.h index 068d53743..12c58ce12 100644 --- a/cpp/common/status_util.h +++ b/cpp/common/status_util.h @@ -5,6 +5,7 @@ #include "absl/base/optimization.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "cerrno" #include "common/macro_utils.h" // This header provides a few utility macros for dealing with absl::Status and @@ -33,12 +34,36 @@ template absl::Status GetStatus(absl::StatusOr status) { return status.status(); } - } // namespace testing::internal -// Alias for a StatusOr that can be used with reference types. +// Get status based on status code and error code. If error code is not 0, then +// corresponding error message is set and appended to the status message. +inline absl::Status GetStatusWithSystemError(absl::StatusCode code, + int error_code, + std::string_view message) { + if (error_code == 0) { + return {code, message}; + } + return {code, absl::StrCat(message, " Error: ", std::strerror(error_code))}; +} + +// Wrapper around std::reference_wrapper that provides functions to access the +// wrapped value as reference or pointer. +template +class ReferenceWrapper : public std::reference_wrapper { + public: + using std::reference_wrapper::reference_wrapper; + // Returns a reference to the wrapped value. + T& AsReference() const { return this->get(); } + // Returns a pointer to the wrapped value. + T* AsPointer() const { return &AsReference(); } + + operator T*() const { return AsPointer(); } +}; + +// Type definition for a StatusOr that can be used with reference types. template -using StatusOrRef = absl::StatusOr>; +using StatusOrRef = absl::StatusOr>; // The implementation of RETURN_IF_ERROR below, more compact as if it would be // if it would be written inline. diff --git a/cpp/common/status_util_test.cc b/cpp/common/status_util_test.cc index 07ab533ae..235f7d6d5 100644 --- a/cpp/common/status_util_test.cc +++ b/cpp/common/status_util_test.cc @@ -2,6 +2,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "cerrno" #include "common/status_test_util.h" #include "gtest/gtest.h" @@ -10,7 +11,9 @@ namespace { using ::testing::_; using ::testing::IsOk; using ::testing::Not; +using ::testing::StartsWith; using ::testing::StatusIs; +using ::testing::StrEq; absl::Status Ok() { return absl::OkStatus(); } @@ -89,6 +92,34 @@ TEST(StatusMacroTest, AssignOrReturnCanReturnPlainStatus) { StatusIs(absl::StatusCode::kInternal, _)); } +TEST(ReferenceWraperTest, ReferenceAddressesAreEqual) { + int x = 10; + auto wrapper = ReferenceWrapper(x); + EXPECT_EQ(&x, &wrapper.AsReference()); +} + +TEST(ReferenceWraperTest, PointsToSameValue) { + int x = 10; + auto wrapper = ReferenceWrapper(x); + EXPECT_EQ(&x, wrapper.AsPointer()); +} + +TEST(StatusWithSystemErrorTest, HasNoSystemError) { + // make sure the errno is set to zero + auto status = GetStatusWithSystemError(absl::StatusCode::kInvalidArgument, 0, + "Invalid arguments."); + EXPECT_THAT(status, StatusIs(absl::StatusCode::kInvalidArgument, + StrEq("Invalid arguments."))); +} + +TEST(StatusWithSystemErrorTest, HasSystemError) { + auto status = GetStatusWithSystemError(absl::StatusCode::kInternal, ENOENT, + "Internal error."); + // assure that error message is appended. + EXPECT_THAT(status, StatusIs(absl::StatusCode::kInternal, + StartsWith("Internal error. Error:"))); +} + absl::StatusOr> CreatePair() { return std::pair{1, 2}; } absl::StatusOr AssignOrReturnWithDecomposition() { @@ -99,5 +130,4 @@ absl::StatusOr AssignOrReturnWithDecomposition() { TEST(StatusMacroTest, AssignCanHandleDecomposition) { EXPECT_THAT(AssignOrReturnWithDecomposition(), 3); } - } // namespace diff --git a/cpp/common/type.cc b/cpp/common/type.cc deleted file mode 100644 index 093f67c6d..000000000 --- a/cpp/common/type.cc +++ /dev/null @@ -1 +0,0 @@ -namespace carmen {} // namespace carmen diff --git a/cpp/state/BUILD b/cpp/state/BUILD index 5013ec3fc..9e9b07bce 100644 --- a/cpp/state/BUILD +++ b/cpp/state/BUILD @@ -16,6 +16,8 @@ cc_test( deps = [ ":configurations", ":state", + "//common:type", + "//common:account_state", "//common:file_util", "//common:status_test_util", "@com_google_googletest//:gtest_main", diff --git a/cpp/state/c_state.cc b/cpp/state/c_state.cc index e0f5a63d7..cc1f2ddd7 100644 --- a/cpp/state/c_state.cc +++ b/cpp/state/c_state.cc @@ -155,11 +155,17 @@ C_State Carmen_CreateLevelDbBasedState(const char* directory, int length) { } void Carmen_Flush(C_State state) { - reinterpret_cast(state)->Flush().IgnoreError(); + auto res = reinterpret_cast(state)->Flush(); + if (!res.ok()) { + std::cout << "WARNING: Failed to flush state: " << res << "\n"; + } } void Carmen_Close(C_State state) { - reinterpret_cast(state)->Close().IgnoreError(); + auto res = reinterpret_cast(state)->Close(); + if (!res.ok()) { + std::cout << "WARNING: Failed to close state: " << res << "\n"; + } } void Carmen_ReleaseState(C_State state) { diff --git a/cpp/state/c_state.h b/cpp/state/c_state.h index 014f9a4c4..50f91db1b 100644 --- a/cpp/state/c_state.h +++ b/cpp/state/c_state.h @@ -1,5 +1,5 @@ // This header file defines a C interface for manipulating the world state. -// It is intendend to be used to bridge the Go/C++ boundary. +// It is intended to be used to bridge the Go/C++ boundary. #include @@ -15,7 +15,7 @@ extern "C" { // The following macro definitions provide syntactic sugar for type-erased // pointers used in the interface definitions below. Their main purpose is to -// increase readability, not to enforce any type contraints. +// increase readability, not to enforce any type constraints. #define C_State void* @@ -43,11 +43,11 @@ C_State Carmen_CreateFileBasedState(const char* directory, int length); // Creates a new state object maintaining data in a LevelDB instance located in // the given directory and returns an opaque pointer to it. Ownership of the -// state is transfered to the caller, which is required to release it +// state is transferred to the caller, which is required to release it // eventually. C_State Carmen_CreateLevelDbBasedState(const char* directory, int length); -// Flushes all committed state information to disk to gurantee permanent +// Flushes all committed state information to disk to guarantee permanent // storage. All internally cached modifications is synced to disk. void Carmen_Flush(C_State state); diff --git a/cpp/state/state_test.cc b/cpp/state/state_test.cc index a7cb6404f..6df046854 100644 --- a/cpp/state/state_test.cc +++ b/cpp/state/state_test.cc @@ -1,11 +1,7 @@ #include "state/state.h" -#include "backend/depot/memory/depot.h" -#include "backend/index/memory/index.h" -#include "backend/store/memory/store.h" #include "common/account_state.h" #include "common/file_util.h" -#include "common/memory_usage.h" #include "common/status_test_util.h" #include "common/type.h" #include "gmock/gmock.h" @@ -148,8 +144,7 @@ TYPED_TEST_P(StateTest, BalancesAreCoveredByGlobalStateHash) { // Resetting value gets us original hash. EXPECT_OK(state.SetBalance({}, Balance{0x12})); - ASSERT_OK_AND_ASSIGN(auto value_12_hash_again, state.GetHash()); - EXPECT_EQ(value_12_hash, value_12_hash_again); + EXPECT_THAT(state.GetHash(), IsOkAndHolds(value_12_hash)); } TYPED_TEST_P(StateTest, DefaultNonceIsZero) { @@ -195,8 +190,7 @@ TYPED_TEST_P(StateTest, NoncesAreCoveredByGlobalStateHash) { // Resetting value gets us original hash. EXPECT_OK(state.SetNonce({}, Nonce{0x12})); - ASSERT_OK_AND_ASSIGN(auto value_12_hash_again, state.GetHash()); - EXPECT_EQ(value_12_hash, value_12_hash_again); + EXPECT_THAT(state.GetHash(), IsOkAndHolds(value_12_hash)); } TYPED_TEST_P(StateTest, DefaultCodeIsEmpty) { @@ -261,8 +255,7 @@ TYPED_TEST_P(StateTest, CodesAreCoveredByGlobalStateHash) { // Resetting value gets us original hash. EXPECT_OK(state.SetCode({}, std::vector{std::byte{12}})); - ASSERT_OK_AND_ASSIGN(auto value_12_hash_again, state.GetHash()); - EXPECT_EQ(value_12_hash, value_12_hash_again); + EXPECT_THAT(state.GetHash(), IsOkAndHolds(value_12_hash)); } TYPED_TEST_P(StateTest, LookingUpMissingCodeDoesNotChangeGlobalHash) {