From 6a27619544026be64de5eac090764e989c1a35f6 Mon Sep 17 00:00:00 2001 From: sfc-gh-tclinkenbeard Date: Wed, 21 Oct 2020 22:19:15 -0700 Subject: [PATCH] Remove unnecessary copies in BackupContainer code --- fdbclient/BackupContainer.actor.cpp | 4 ++-- fdbclient/BackupContainer.h | 8 ++++---- .../BackupContainerAzureBlobStore.actor.cpp | 6 +++--- fdbclient/BackupContainerAzureBlobStore.h | 6 +++--- fdbclient/BackupContainerFileSystem.actor.cpp | 13 ++++++------ fdbclient/BackupContainerFileSystem.h | 20 +++++++++---------- .../BackupContainerLocalDirectory.actor.cpp | 10 +++++----- fdbclient/BackupContainerLocalDirectory.h | 10 +++++----- .../BackupContainerS3BlobStore.actor.cpp | 12 +++++------ fdbclient/BackupContainerS3BlobStore.h | 12 +++++------ 10 files changed, 50 insertions(+), 51 deletions(-) diff --git a/fdbclient/BackupContainer.actor.cpp b/fdbclient/BackupContainer.actor.cpp index 01154592eb..2a91ddaa21 100644 --- a/fdbclient/BackupContainer.actor.cpp +++ b/fdbclient/BackupContainer.actor.cpp @@ -248,7 +248,7 @@ std::vector IBackupContainer::getURLFormats() { } // Get an IBackupContainer based on a container URL string -Reference IBackupContainer::openContainer(std::string url) { +Reference IBackupContainer::openContainer(const std::string& url) { static std::map> m_cache; Reference& r = m_cache[url]; @@ -338,7 +338,7 @@ ACTOR Future> listContainers_impl(std::string baseURL) } } -Future> IBackupContainer::listContainers(std::string baseURL) { +Future> IBackupContainer::listContainers(const std::string& baseURL) { return listContainers_impl(baseURL); } diff --git a/fdbclient/BackupContainer.h b/fdbclient/BackupContainer.h index f346ebd44d..c82c0205ef 100644 --- a/fdbclient/BackupContainer.h +++ b/fdbclient/BackupContainer.h @@ -40,7 +40,7 @@ Future timeKeeperVersionFromDatetime(std::string const &datetime, Datab // TODO: Move the log file and range file format encoding/decoding stuff to this file and behind interfaces. class IBackupFile { public: - IBackupFile(std::string fileName) : m_fileName(fileName), m_offset(0) {} + IBackupFile(const std::string& fileName) : m_fileName(fileName), m_offset(0) {} virtual ~IBackupFile() {} // Backup files are append-only and cannot have more than 1 append outstanding at once. virtual Future append(const void *data, int len) = 0; @@ -247,7 +247,7 @@ public: int64_t totalBytes) = 0; // Open a file for read by name - virtual Future> readFile(std::string name) = 0; + virtual Future> readFile(const std::string& name) = 0; // Returns the key ranges in the snapshot file. This is an expensive function // and should only be used in simulation for sanity check. @@ -289,9 +289,9 @@ public: bool logsOnly = false, Version beginVersion = -1) = 0; // Get an IBackupContainer based on a container spec string - static Reference openContainer(std::string url); + static Reference openContainer(const std::string& url); static std::vector getURLFormats(); - static Future> listContainers(std::string baseURL); + static Future> listContainers(const std::string& baseURL); std::string getURL() const { return URL; diff --git a/fdbclient/BackupContainerAzureBlobStore.actor.cpp b/fdbclient/BackupContainerAzureBlobStore.actor.cpp index 131a743fc1..35800c912d 100644 --- a/fdbclient/BackupContainerAzureBlobStore.actor.cpp +++ b/fdbclient/BackupContainerAzureBlobStore.actor.cpp @@ -247,7 +247,7 @@ Future BackupContainerAzureBlobStore::exists() { }); } -Future> BackupContainerAzureBlobStore::readFile(std::string fileName) { +Future> BackupContainerAzureBlobStore::readFile(const std::string& fileName) { return BackupContainerAzureBlobStoreImpl::readFile(this, fileName); } @@ -256,7 +256,7 @@ Future> BackupContainerAzureBlobStore::writeFile(const st } Future BackupContainerAzureBlobStore::listFiles( - std::string path, std::function folderPathFilter) { + const std::string& path, std::function folderPathFilter) { return asyncTaskThread.execAsync([client = this->client.get(), containerName = this->containerName, path = path, folderPathFilter = folderPathFilter] { FilesAndSizesT result; @@ -265,7 +265,7 @@ Future BackupContainerAzureBlobStore: }); } -Future BackupContainerAzureBlobStore::deleteFile(std::string fileName) { +Future BackupContainerAzureBlobStore::deleteFile(const std::string& fileName) { return asyncTaskThread.execAsync( [containerName = this->containerName, fileName = fileName, client = client.get()]() { client->delete_blob(containerName, fileName).wait(); diff --git a/fdbclient/BackupContainerAzureBlobStore.h b/fdbclient/BackupContainerAzureBlobStore.h index 646e3f2857..afe6690f61 100644 --- a/fdbclient/BackupContainerAzureBlobStore.h +++ b/fdbclient/BackupContainerAzureBlobStore.h @@ -50,14 +50,14 @@ public: Future exists() override; - Future> readFile(std::string fileName) override; + Future> readFile(const std::string& fileName) override; Future> writeFile(const std::string& fileName) override; - Future listFiles(std::string path = "", + Future listFiles(const std::string& path = "", std::function folderPathFilter = nullptr) override; - Future deleteFile(std::string fileName) override; + Future deleteFile(const std::string& fileName) override; Future deleteContainer(int* pNumDeleted) override; }; diff --git a/fdbclient/BackupContainerFileSystem.actor.cpp b/fdbclient/BackupContainerFileSystem.actor.cpp index cd4e2ccb8b..e023c20802 100644 --- a/fdbclient/BackupContainerFileSystem.actor.cpp +++ b/fdbclient/BackupContainerFileSystem.actor.cpp @@ -734,7 +734,7 @@ std::string BackupContainerFileSystem::snapshotFolderString(Version snapshotBegi return format("kvranges/snapshot.%018" PRId64, snapshotBeginVersion); } -Version BackupContainerFileSystem::extractSnapshotBeginVersion(std::string path) { +Version BackupContainerFileSystem::extractSnapshotBeginVersion(const std::string& path) { Version snapshotBeginVersion; if (sscanf(path.c_str(), "kvranges/snapshot.%018" SCNd64, &snapshotBeginVersion) == 1) { return snapshotBeginVersion; @@ -776,7 +776,7 @@ Future> BackupContainerFileSystem::writeRangeFile(Version format("/%d/", snapshotFileCount / (BUGGIFY ? 1 : 5000)) + fileName); } -std::string BackupContainerFileSystem::fileNameOnly(std::string path) { +std::string BackupContainerFileSystem::fileNameOnly(const std::string& path) { // Find the last forward slash position, defaulting to 0 if not found int pos = path.find_last_of('/'); if (pos == std::string::npos) { @@ -790,7 +790,7 @@ std::string BackupContainerFileSystem::fileNameOnly(std::string path) { return path.substr(pos + 1); } -bool BackupContainerFileSystem::pathToRangeFile(RangeFile& out, std::string path, int64_t size) { +bool BackupContainerFileSystem::pathToRangeFile(RangeFile& out, const std::string& path, int64_t size) { std::string name = fileNameOnly(path); RangeFile f; f.fileName = path; @@ -804,7 +804,7 @@ bool BackupContainerFileSystem::pathToRangeFile(RangeFile& out, std::string path return false; } -bool BackupContainerFileSystem::pathToLogFile(LogFile& out, std::string path, int64_t size) { +bool BackupContainerFileSystem::pathToLogFile(LogFile& out, const std::string& path, int64_t size) { std::string name = fileNameOnly(path); LogFile f; f.fileName = path; @@ -824,7 +824,7 @@ bool BackupContainerFileSystem::pathToLogFile(LogFile& out, std::string path, in return false; } -bool BackupContainerFileSystem::pathToKeyspaceSnapshotFile(KeyspaceSnapshotFile& out, std::string path) { +bool BackupContainerFileSystem::pathToKeyspaceSnapshotFile(KeyspaceSnapshotFile& out, const std::string& path) { std::string name = fileNameOnly(path); KeyspaceSnapshotFile f; f.fileName = path; @@ -1264,7 +1264,7 @@ Future BackupContainerFileSystem::getSnapshotFileKeyRange(const RangeF return getSnapshotFileKeyRange_impl(Reference::addRef(this), file); } -Optional BackupContainerFileSystem::getRestoreSetFromLogs(std::vector logs, +Optional BackupContainerFileSystem::getRestoreSetFromLogs(const std::vector& logs, Version targetVersion, RestorableFileSet restorable) { Version end = logs.begin()->endVersion; @@ -1277,7 +1277,6 @@ Optional BackupContainerFileSystem::getRestoreSetFromLogs(std return Optional(); } - Future> BackupContainerFileSystem::getRestoreSet(Version targetVersion, VectorRef keyRangesFilter, bool logsOnly, Version beginVersion) { diff --git a/fdbclient/BackupContainerFileSystem.h b/fdbclient/BackupContainerFileSystem.h index 090c395486..80dbaec949 100644 --- a/fdbclient/BackupContainerFileSystem.h +++ b/fdbclient/BackupContainerFileSystem.h @@ -103,17 +103,17 @@ public: // Although not required, an implementation can avoid traversing unwanted subfolders // by calling folderPathFilter(absoluteFolderPath) and checking for a false return value. using FilesAndSizesT = std::vector>; - virtual Future listFiles(std::string path = "", + virtual Future listFiles(const std::string& path = "", std::function folderPathFilter = nullptr) = 0; // Open a file for read by fileName - Future> readFile(std::string fileName) override = 0; + Future> readFile(const std::string& fileName) override = 0; // Open a file for write by fileName virtual Future> writeFile(const std::string& fileName) = 0; // Delete a file - virtual Future deleteFile(std::string fileName) = 0; + virtual Future deleteFile(const std::string& fileName) = 0; // Delete entire container. During the process, if pNumDeleted is not null it will be // updated with the count of deleted files so that progress can be seen. @@ -134,7 +134,7 @@ public: static std::string snapshotFolderString(Version snapshotBeginVersion); // Extract the snapshot begin version from a path - static Version extractSnapshotBeginVersion(std::string path); + static Version extractSnapshotBeginVersion(const std::string& path); // The innermost folder covers 100,000 seconds (1e11 versions) which is 5,000 mutation log files at current // settings. @@ -150,13 +150,13 @@ public: // Find what should be the filename of a path by finding whatever is after the last forward or backward slash, or // failing to find those, the whole string. - static std::string fileNameOnly(std::string path); + static std::string fileNameOnly(const std::string& path); - static bool pathToRangeFile(RangeFile& out, std::string path, int64_t size); + static bool pathToRangeFile(RangeFile& out, const std::string& path, int64_t size); - static bool pathToLogFile(LogFile& out, std::string path, int64_t size); + static bool pathToLogFile(LogFile& out, const std::string& path, int64_t size); - static bool pathToKeyspaceSnapshotFile(KeyspaceSnapshotFile& out, std::string path); + static bool pathToKeyspaceSnapshotFile(KeyspaceSnapshotFile& out, const std::string& path); Future, std::map>> readKeyspaceSnapshot( KeyspaceSnapshotFile snapshot); @@ -233,7 +233,7 @@ public: Future getSnapshotFileKeyRange(const RangeFile& file) final; - static Optional getRestoreSetFromLogs(std::vector logs, Version targetVersion, + static Optional getRestoreSetFromLogs(const std::vector& logs, Version targetVersion, RestorableFileSet restorable); Future> getRestoreSet(Version targetVersion, VectorRef keyRangesFilter, @@ -241,7 +241,7 @@ public: private: struct VersionProperty { - VersionProperty(Reference bc, std::string name) + VersionProperty(Reference bc, const std::string& name) : bc(bc), path("properties/" + name) {} Reference bc; std::string path; diff --git a/fdbclient/BackupContainerLocalDirectory.actor.cpp b/fdbclient/BackupContainerLocalDirectory.actor.cpp index 9b2a976376..d76dcd18cd 100644 --- a/fdbclient/BackupContainerLocalDirectory.actor.cpp +++ b/fdbclient/BackupContainerLocalDirectory.actor.cpp @@ -91,7 +91,7 @@ std::string BackupContainerLocalDirectory::getURLFormat() { return "file://"; } -BackupContainerLocalDirectory::BackupContainerLocalDirectory(std::string url) { +BackupContainerLocalDirectory::BackupContainerLocalDirectory(const std::string& url) { std::string path; if (url.find("file://") != 0) { TraceEvent(SevWarn, "BackupContainerLocalDirectory") @@ -121,7 +121,7 @@ BackupContainerLocalDirectory::BackupContainerLocalDirectory(std::string url) { m_path = path; } -Future> BackupContainerLocalDirectory::listURLs(std::string url) { +Future> BackupContainerLocalDirectory::listURLs(const std::string& url) { std::string path; if (url.find("file://") != 0) { TraceEvent(SevWarn, "BackupContainerLocalDirectory") @@ -164,7 +164,7 @@ Future BackupContainerLocalDirectory::exists() { return directoryExists(m_path); } -Future> BackupContainerLocalDirectory::readFile(std::string path) { +Future> BackupContainerLocalDirectory::readFile(const std::string& path) { int flags = IAsyncFile::OPEN_NO_AIO | IAsyncFile::OPEN_READONLY | IAsyncFile::OPEN_UNCACHED; // Simulation does not properly handle opening the same file from multiple machines using a shared filesystem, // so create a symbolic link to make each file opening appear to be unique. This could also work in production @@ -226,13 +226,13 @@ Future> BackupContainerLocalDirectory::writeFile(const st return map(f, [=](Reference f) { return Reference(new BackupFile(path, f, fullPath)); }); } -Future BackupContainerLocalDirectory::deleteFile(std::string path) { +Future BackupContainerLocalDirectory::deleteFile(const std::string& path) { ::deleteFile(joinPath(m_path, path)); return Void(); } Future BackupContainerLocalDirectory::listFiles( - std::string path, std::function) { + const std::string& path, std::function) { return listFiles_impl(path, m_path); } diff --git a/fdbclient/BackupContainerLocalDirectory.h b/fdbclient/BackupContainerLocalDirectory.h index c19dd4fa1b..4bb8c2cb78 100644 --- a/fdbclient/BackupContainerLocalDirectory.h +++ b/fdbclient/BackupContainerLocalDirectory.h @@ -32,22 +32,22 @@ public: static std::string getURLFormat(); - BackupContainerLocalDirectory(std::string url); + BackupContainerLocalDirectory(const std::string& url); - static Future> listURLs(std::string url); + static Future> listURLs(const std::string& url); Future create() final; // The container exists if the folder it resides in exists Future exists() final; - Future> readFile(std::string path) final; + Future> readFile(const std::string& path) final; Future> writeFile(const std::string& path) final; - Future deleteFile(std::string path) final; + Future deleteFile(const std::string& path) final; - Future listFiles(std::string path, std::function) final; + Future listFiles(const std::string& path, std::function) final; Future deleteContainer(int* pNumDeleted) final; diff --git a/fdbclient/BackupContainerS3BlobStore.actor.cpp b/fdbclient/BackupContainerS3BlobStore.actor.cpp index 7f91cb383f..80ae5577a2 100644 --- a/fdbclient/BackupContainerS3BlobStore.actor.cpp +++ b/fdbclient/BackupContainerS3BlobStore.actor.cpp @@ -120,7 +120,7 @@ public: const std::string BackupContainerS3BlobStoreImpl::DATAFOLDER = "data"; const std::string BackupContainerS3BlobStoreImpl::INDEXFOLDER = "backups"; -std::string BackupContainerS3BlobStore::dataPath(const std::string path) { +std::string BackupContainerS3BlobStore::dataPath(const std::string& path) { return BackupContainerS3BlobStoreImpl::DATAFOLDER + "/" + m_name + "/" + path; } @@ -129,7 +129,7 @@ std::string BackupContainerS3BlobStore::indexEntry() { return BackupContainerS3BlobStoreImpl::INDEXFOLDER + "/" + m_name; } -BackupContainerS3BlobStore::BackupContainerS3BlobStore(Reference bstore, std::string name, +BackupContainerS3BlobStore::BackupContainerS3BlobStore(Reference bstore, const std::string& name, const BlobStoreEndpoint::ParametersT& params) : m_bstore(bstore), m_name(name), m_bucket("FDB_BACKUPS_V2") { @@ -158,7 +158,7 @@ std::string BackupContainerS3BlobStore::getURLFormat() { return BlobStoreEndpoint::getURLFormat(true) + " (Note: The 'bucket' parameter is required.)"; } -Future> BackupContainerS3BlobStore::readFile(std::string path) { +Future> BackupContainerS3BlobStore::readFile(const std::string& path) { return Reference(new AsyncFileReadAheadCache( Reference(new AsyncFileBlobStoreRead(m_bstore, m_bucket, dataPath(path))), m_bstore->knobs.read_block_size, m_bstore->knobs.read_ahead_blocks, m_bstore->knobs.concurrent_reads_per_file, @@ -166,7 +166,7 @@ Future> BackupContainerS3BlobStore::readFile(std::string p } Future> BackupContainerS3BlobStore::listURLs(Reference bstore, - std::string bucket) { + const std::string& bucket) { return BackupContainerS3BlobStoreImpl::listURLs(bstore, bucket); } @@ -175,12 +175,12 @@ Future> BackupContainerS3BlobStore::writeFile(const std:: path, Reference(new AsyncFileBlobStoreWrite(m_bstore, m_bucket, dataPath(path))))); } -Future BackupContainerS3BlobStore::deleteFile(std::string path) { +Future BackupContainerS3BlobStore::deleteFile(const std::string& path) { return m_bstore->deleteObject(m_bucket, dataPath(path)); } Future BackupContainerS3BlobStore::listFiles( - std::string path, std::function pathFilter) { + const std::string& path, std::function pathFilter) { return BackupContainerS3BlobStoreImpl::listFiles(Reference::addRef(this), path, pathFilter); } diff --git a/fdbclient/BackupContainerS3BlobStore.h b/fdbclient/BackupContainerS3BlobStore.h index 3ee5a6e02e..0d5f8ae8a9 100644 --- a/fdbclient/BackupContainerS3BlobStore.h +++ b/fdbclient/BackupContainerS3BlobStore.h @@ -32,7 +32,7 @@ class BackupContainerS3BlobStore final : public BackupContainerFileSystem, // All backup data goes into a single bucket std::string m_bucket; - std::string dataPath(const std::string path); + std::string dataPath(const std::string& path); // Get the path of the backups's index entry std::string indexEntry(); @@ -40,7 +40,7 @@ class BackupContainerS3BlobStore final : public BackupContainerFileSystem, friend class BackupContainerS3BlobStoreImpl; public: - BackupContainerS3BlobStore(Reference bstore, std::string name, + BackupContainerS3BlobStore(Reference bstore, const std::string& name, const BlobStoreEndpoint::ParametersT& params); void addref() override; @@ -48,15 +48,15 @@ public: static std::string getURLFormat(); - Future> readFile(std::string path) final; + Future> readFile(const std::string& path) final; - static Future> listURLs(Reference bstore, std::string bucket); + static Future> listURLs(Reference bstore, const std::string& bucket); Future> writeFile(const std::string& path) final; - Future deleteFile(std::string path) final; + Future deleteFile(const std::string& path) final; - Future listFiles(std::string path, std::function pathFilter) final; + Future listFiles(const std::string& path, std::function pathFilter) final; Future create() final;