From 5dd08d6522896553d71ce89f62c367cdfa693f53 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Thu, 19 Dec 2024 17:32:25 +0100 Subject: [PATCH 1/4] Add tests for InputFileCatalog --- FWCore/Catalog/test/BuildFile.xml | 2 +- FWCore/Catalog/test/FileLocator_t.cpp | 94 ++---------- FWCore/Catalog/test/InputFileCatalog_t.cpp | 163 +++++++++++++++++++++ FWCore/Catalog/test/TestSiteLocalConfig.h | 87 +++++++++++ 4 files changed, 262 insertions(+), 84 deletions(-) create mode 100644 FWCore/Catalog/test/InputFileCatalog_t.cpp create mode 100644 FWCore/Catalog/test/TestSiteLocalConfig.h diff --git a/FWCore/Catalog/test/BuildFile.xml b/FWCore/Catalog/test/BuildFile.xml index 484a7cb422667..e1e65241d3ebf 100644 --- a/FWCore/Catalog/test/BuildFile.xml +++ b/FWCore/Catalog/test/BuildFile.xml @@ -1,5 +1,5 @@ - + diff --git a/FWCore/Catalog/test/FileLocator_t.cpp b/FWCore/Catalog/test/FileLocator_t.cpp index 3da5d9c616f80..d71c20c84e26a 100644 --- a/FWCore/Catalog/test/FileLocator_t.cpp +++ b/FWCore/Catalog/test/FileLocator_t.cpp @@ -2,96 +2,24 @@ #include "FWCore/Catalog/interface/SiteLocalConfig.h" #include "FWCore/Utilities/interface/Exception.h" #include "FWCore/ServiceRegistry/interface/ServiceRegistry.h" -#include +#include "TestSiteLocalConfig.h" + +#include #include #define CATCH_CONFIG_MAIN #include "catch.hpp" -namespace { - class TestSiteLocalConfig : public edm::SiteLocalConfig { - public: - //constructor using trivial data catalogs - TestSiteLocalConfig(std::vector catalogs) : m_trivialCatalogs(std::move(catalogs)) {} - //constructor using Rucio data catalogs - TestSiteLocalConfig(std::vector catalogs) : m_catalogs(std::move(catalogs)) {} - std::vector const& trivialDataCatalogs() const final { return m_trivialCatalogs; } - std::vector const& dataCatalogs() const final { return m_catalogs; } - std::filesystem::path const storageDescriptionPath(const edm::CatalogAttributes& aDataCatalog) const final { - return std::filesystem::path(); - } - - std::string const lookupCalibConnect(std::string const& input) const final { return std::string(); } - std::string const rfioType(void) const final { return std::string(); } - - std::string const* sourceCacheTempDir() const final { return nullptr; } - double const* sourceCacheMinFree() const final { return nullptr; } - std::string const* sourceCacheHint() const final { return nullptr; } - std::string const* sourceCloneCacheHint() const final { return nullptr; } - std::string const* sourceReadHint() const final { return nullptr; } - unsigned int const* sourceTTreeCacheSize() const final { return nullptr; } - unsigned int const* sourceTimeout() const final { return nullptr; } - bool enablePrefetching() const final { return false; } - unsigned int debugLevel() const final { return 0; } - std::vector const* sourceNativeProtocols() const final { return nullptr; } - struct addrinfo const* statisticsDestination() const final { return nullptr; } - std::set const* statisticsInfo() const final { return nullptr; } - std::string const& siteName(void) const final { return m_emptyString; } - std::string const& subSiteName(void) const final { return m_emptyString; } - bool useLocalConnectString() const final { return false; } - std::string const& localConnectPrefix() const final { return m_emptyString; } - std::string const& localConnectSuffix() const final { return m_emptyString; } - - private: - std::vector m_trivialCatalogs; - std::vector m_catalogs; - std::filesystem::path m_storageDescription_path; - std::string m_emptyString; - }; -} // namespace - -TEST_CASE("FileLocator with Rucio data catalog", "[filelocatorRucioDataCatalog]") { - //catalog for testing "prefix" - edm::CatalogAttributes aCatalog; - aCatalog.site = "T1_US_FNAL"; - aCatalog.subSite = "T1_US_FNAL"; - aCatalog.storageSite = "T1_US_FNAL"; - aCatalog.volume = "American_Federation"; - aCatalog.protocol = "XRootD"; - std::vector tmp{aCatalog}; - //catalog for testing "rules" - aCatalog.site = "T1_US_FNAL"; - aCatalog.subSite = "T1_US_FNAL"; - aCatalog.storageSite = "T1_US_FNAL"; - aCatalog.volume = "FNAL_dCache_EOS"; - aCatalog.protocol = "XRootD"; - tmp.push_back(aCatalog); - //catalog for testing chained "rules" - aCatalog.site = "T1_US_FNAL"; - aCatalog.subSite = "T1_US_FNAL"; - aCatalog.storageSite = "T1_US_FNAL"; - aCatalog.volume = "FNAL_dCache_EOS"; - aCatalog.protocol = "root"; - tmp.push_back(aCatalog); - - //create the services - edm::ServiceToken tempToken( - edm::ServiceRegistry::createContaining(std::unique_ptr(new TestSiteLocalConfig(tmp)))); - - std::string CMSSW_BASE(std::getenv("CMSSW_BASE")); - std::string CMSSW_RELEASE_BASE(std::getenv("CMSSW_RELEASE_BASE")); - std::string file_name("/src/FWCore/Catalog/test/storage.json"); - std::string full_file_name = std::filesystem::exists((CMSSW_BASE + file_name).c_str()) - ? CMSSW_BASE + file_name - : CMSSW_RELEASE_BASE + file_name; +TEST_CASE("FileLocator with Rucio data catalog", "[FWCore/Catalog]") { + edm::ServiceToken tempToken = edmtest::catalog::makeTestSiteLocalConfigToken(); SECTION("prefix") { edm::ServiceRegistry::Operate operate(tempToken); //empty catalog edm::CatalogAttributes tmp_cat; //use the first catalog provided by site local config - edm::FileLocator fl(tmp_cat, 0, full_file_name); + edm::FileLocator fl(tmp_cat, 0); CHECK("root://cmsxrootd.fnal.gov/store/group/bha/bho" == fl.pfn("/store/group/bha/bho", edm::CatalogType::RucioCatalog)); } @@ -100,7 +28,7 @@ TEST_CASE("FileLocator with Rucio data catalog", "[filelocatorRucioDataCatalog]" //empty catalog edm::CatalogAttributes tmp_cat; //use the second catalog provided by site local config - edm::FileLocator fl(tmp_cat, 1, full_file_name); + edm::FileLocator fl(tmp_cat, 1); const std::array lfn = {{"/bha/bho", "bha", "file:bha", @@ -119,7 +47,7 @@ TEST_CASE("FileLocator with Rucio data catalog", "[filelocatorRucioDataCatalog]" //empty catalog edm::CatalogAttributes tmp_cat; //use the third catalog provided by site local config above - edm::FileLocator fl(tmp_cat, 2, full_file_name); + edm::FileLocator fl(tmp_cat, 2); const std::array lfn = {{"/bha/bho", "bha", "file:bha", @@ -139,7 +67,7 @@ TEST_CASE("FileLocator with Rucio data catalog", "[filelocatorRucioDataCatalog]" } } -TEST_CASE("FileLocator", "[filelocator]") { +TEST_CASE("FileLocator with TrivialFileCatalog", "[FWCore/Catalog]") { std::string CMSSW_BASE(std::getenv("CMSSW_BASE")); std::string CMSSW_RELEASE_BASE(std::getenv("CMSSW_RELEASE_BASE")); std::string file_name("/src/FWCore/Catalog/test/simple_catalog.xml"); @@ -149,8 +77,8 @@ TEST_CASE("FileLocator", "[filelocator]") { //create the services std::vector tmp{std::string("trivialcatalog_file:") + full_file_name + "?protocol=xrd"}; - edm::ServiceToken tempToken( - edm::ServiceRegistry::createContaining(std::unique_ptr(new TestSiteLocalConfig(tmp)))); + edm::ServiceToken tempToken(edm::ServiceRegistry::createContaining( + std::unique_ptr(std::make_unique(tmp)))); //make the services available SECTION("standard") { diff --git a/FWCore/Catalog/test/InputFileCatalog_t.cpp b/FWCore/Catalog/test/InputFileCatalog_t.cpp new file mode 100644 index 0000000000000..52edef9f9d3bc --- /dev/null +++ b/FWCore/Catalog/test/InputFileCatalog_t.cpp @@ -0,0 +1,163 @@ +#include "FWCore/Catalog/interface/InputFileCatalog.h" +#include "FWCore/ServiceRegistry/interface/ServiceRegistry.h" + +#include "TestSiteLocalConfig.h" + +#include "catch.hpp" + +TEST_CASE("InputFileCatalog with Rucio data catalog", "[FWCore/Catalog]") { + edm::ServiceToken tempToken = edmtest::catalog::makeTestSiteLocalConfigToken(); + + SECTION("Empty") { + edm::ServiceRegistry::Operate operate(tempToken); + edm::InputFileCatalog catalog({}, ""); + REQUIRE(catalog.empty()); + } + + SECTION("isPhysical") { + REQUIRE(edm::InputFileCatalog::isPhysical("")); + REQUIRE(not edm::InputFileCatalog::isPhysical("/foo/bar")); + REQUIRE(edm::InputFileCatalog::isPhysical("file:/foo/bar")); + } + + SECTION("Default behavior") { + edm::ServiceRegistry::Operate operate(tempToken); + + edm::InputFileCatalog catalog(std::vector{"/store/foo/bar", " file:/foo/bar", "root://foobar "}, ""); + + SECTION("logicalFileNames") { + auto const& lfns = catalog.logicalFileNames(); + REQUIRE(lfns.size() == 3); + CHECK(lfns[0] == "/store/foo/bar"); + CHECK(lfns[1] == ""); // was PFN + CHECK(lfns[2] == ""); // was PFN + } + + SECTION("fileNames") { + SECTION("Catalog 0") { + auto const names = catalog.fileNames(0); + REQUIRE(names.size() == 3); + CHECK(names[0] == "root://cmsxrootd.fnal.gov/store/foo/bar"); + CHECK(names[1] == "file:/foo/bar"); + CHECK(names[2] == "root://foobar"); + } + // The fileNames() works only for catalog 0 + // Catalog 1 or 2 leads to a crash because the input file list + // is a mixture of LFNs and PFNs + // This isn't really good behavior... + } + + SECTION("fileCatalogItems") { + auto const& items = catalog.fileCatalogItems(); + REQUIRE(items.size() == 3); + CHECK(items[0].logicalFileName() == "/store/foo/bar"); + CHECK(items[1].logicalFileName() == ""); + CHECK(items[2].logicalFileName() == ""); + + REQUIRE(items[0].fileNames().size() == 3); + REQUIRE(items[1].fileNames().size() == 1); + REQUIRE(items[2].fileNames().size() == 1); + + SECTION("Catalog 0") { + CHECK(items[0].fileName(0) == "root://cmsxrootd.fnal.gov/store/foo/bar"); + CHECK(items[1].fileName(0) == "file:/foo/bar"); + CHECK(items[2].fileName(0) == "root://foobar"); + } + SECTION("Catalog 1") { + CHECK(items[0].fileName(1) == "root://cmsdcadisk.fnal.gov//dcache/uscmsdisk/store/foo/bar"); + } + SECTION("Catalog 2") { CHECK(items[0].fileName(2) == "root://host.domain//pnfs/cms/store/foo/bar"); } + } + } + + SECTION("Override catalog") { + edm::ServiceRegistry::Operate operate(tempToken); + + edm::InputFileCatalog catalog(std::vector{"/store/foo/bar", " file:/foo/bar", "root://foobar "}, + "T1_US_FNAL,,T1_US_FNAL,FNAL_dCache_EOS,XRootD"); + + SECTION("logicalFileNames") { + auto const& lfns = catalog.logicalFileNames(); + REQUIRE(lfns.size() == 3); + CHECK(lfns[0] == "/store/foo/bar"); + CHECK(lfns[1] == ""); // was PFN + CHECK(lfns[2] == ""); // was PFN + } + + SECTION("fileNames") { + auto const names = catalog.fileNames(0); + REQUIRE(names.size() == 3); + CHECK(names[0] == "root://cmsdcadisk.fnal.gov//dcache/uscmsdisk/store/foo/bar"); + CHECK(names[1] == "file:/foo/bar"); + CHECK(names[2] == "root://foobar"); + } + + SECTION("fileCatalogItems") { + auto const& items = catalog.fileCatalogItems(); + REQUIRE(items.size() == 3); + + CHECK(items[0].logicalFileName() == "/store/foo/bar"); + CHECK(items[1].logicalFileName() == ""); + CHECK(items[2].logicalFileName() == ""); + + REQUIRE(items[0].fileNames().size() == 1); + REQUIRE(items[1].fileNames().size() == 1); + REQUIRE(items[2].fileNames().size() == 1); + + CHECK(items[0].fileName(0) == "root://cmsdcadisk.fnal.gov//dcache/uscmsdisk/store/foo/bar"); + CHECK(items[1].fileName(0) == "file:/foo/bar"); + CHECK(items[2].fileName(0) == "root://foobar"); + } + } + + SECTION("useLFNasPFNifLFNnotFound") { + edm::ServiceRegistry::Operate operate(tempToken); + + edm::InputFileCatalog catalog( + std::vector{"/store/foo/bar", "/tmp/foo/bar", "root://foobar "}, "", true); + + SECTION("logicalFileNames") { + auto const& lfns = catalog.logicalFileNames(); + REQUIRE(lfns.size() == 3); + CHECK(lfns[0] == "/store/foo/bar"); + CHECK(lfns[1] == "/tmp/foo/bar"); + CHECK(lfns[2] == ""); // was PFN + } + + SECTION("fileNames") { + SECTION("Catalog 0") { + auto const names = catalog.fileNames(0); + REQUIRE(names.size() == 3); + CHECK(names[0] == "root://cmsxrootd.fnal.gov/store/foo/bar"); + CHECK(names[1] == "/tmp/foo/bar"); + CHECK(names[2] == "root://foobar"); + } + } + + SECTION("fileCatalogItems") { + auto const& items = catalog.fileCatalogItems(); + REQUIRE(items.size() == 3); + CHECK(items[0].logicalFileName() == "/store/foo/bar"); + CHECK(items[1].logicalFileName() == "/tmp/foo/bar"); + CHECK(items[2].logicalFileName() == ""); + + REQUIRE(items[0].fileNames().size() == 3); + REQUIRE(items[1].fileNames().size() == 3); + REQUIRE(items[2].fileNames().size() == 1); + + SECTION("Catalog 0") { + CHECK(items[0].fileName(0) == "root://cmsxrootd.fnal.gov/store/foo/bar"); + CHECK(items[1].fileName(0) == "/tmp/foo/bar"); + CHECK(items[2].fileName(0) == "root://foobar"); + } + SECTION("Catalog 1") { + CHECK(items[0].fileName(1) == "root://cmsdcadisk.fnal.gov//dcache/uscmsdisk/store/foo/bar"); + CHECK(items[1].fileName(1) == "/tmp/foo/bar"); + } + SECTION("Catalog 2") { + CHECK(items[0].fileName(2) == "root://host.domain//pnfs/cms/store/foo/bar"); + CHECK(items[1].fileName(2) == "/tmp/foo/bar"); + } + } + } +} diff --git a/FWCore/Catalog/test/TestSiteLocalConfig.h b/FWCore/Catalog/test/TestSiteLocalConfig.h new file mode 100644 index 0000000000000..e9d3ac456e5e3 --- /dev/null +++ b/FWCore/Catalog/test/TestSiteLocalConfig.h @@ -0,0 +1,87 @@ +#ifndef FWCore_Catalog_test_TestSiteLocalConfig_h +#define FWCore_Catalog_test_TestSiteLocalConfig_h + +#include "FWCore/Catalog/interface/SiteLocalConfig.h" + +#include +#include +#include + +namespace edmtest::catalog { + class TestSiteLocalConfig : public edm::SiteLocalConfig { + public: + //constructor using trivial data catalogs + TestSiteLocalConfig(std::vector catalogs) : m_trivialCatalogs(std::move(catalogs)) {} + //constructor using Rucio data catalogs + TestSiteLocalConfig(std::vector catalogs) : m_catalogs(std::move(catalogs)) {} + std::vector const& trivialDataCatalogs() const final { return m_trivialCatalogs; } + std::vector const& dataCatalogs() const final { return m_catalogs; } + std::filesystem::path const storageDescriptionPath(const edm::CatalogAttributes& aDataCatalog) const final { + std::string CMSSW_BASE(std::getenv("CMSSW_BASE")); + std::string CMSSW_RELEASE_BASE(std::getenv("CMSSW_RELEASE_BASE")); + std::string file_name("/src/FWCore/Catalog/test/storage.json"); + std::string full_file_name = std::filesystem::exists((CMSSW_BASE + file_name).c_str()) + ? CMSSW_BASE + file_name + : CMSSW_RELEASE_BASE + file_name; + return full_file_name; + } + + std::string const lookupCalibConnect(std::string const& input) const final { return std::string(); } + std::string const rfioType(void) const final { return std::string(); } + + std::string const* sourceCacheTempDir() const final { return nullptr; } + double const* sourceCacheMinFree() const final { return nullptr; } + std::string const* sourceCacheHint() const final { return nullptr; } + std::string const* sourceCloneCacheHint() const final { return nullptr; } + std::string const* sourceReadHint() const final { return nullptr; } + unsigned int const* sourceTTreeCacheSize() const final { return nullptr; } + unsigned int const* sourceTimeout() const final { return nullptr; } + bool enablePrefetching() const final { return false; } + unsigned int debugLevel() const final { return 0; } + std::vector const* sourceNativeProtocols() const final { return nullptr; } + struct addrinfo const* statisticsDestination() const final { return nullptr; } + std::set const* statisticsInfo() const final { return nullptr; } + std::string const& siteName(void) const final { return m_emptyString; } + std::string const& subSiteName(void) const final { return m_emptyString; } + bool useLocalConnectString() const final { return false; } + std::string const& localConnectPrefix() const final { return m_emptyString; } + std::string const& localConnectSuffix() const final { return m_emptyString; } + + private: + std::vector m_trivialCatalogs; + std::vector m_catalogs; + std::filesystem::path m_storageDescription_path; + std::string m_emptyString; + }; + + inline edm::ServiceToken makeTestSiteLocalConfigToken() { + //catalog for testing "prefix" + edm::CatalogAttributes aCatalog; + aCatalog.site = "T1_US_FNAL"; + aCatalog.subSite = "T1_US_FNAL"; + aCatalog.storageSite = "T1_US_FNAL"; + aCatalog.volume = "American_Federation"; + aCatalog.protocol = "XRootD"; + std::vector tmp{aCatalog}; + //catalog for testing "rules" + aCatalog.site = "T1_US_FNAL"; + aCatalog.subSite = "T1_US_FNAL"; + aCatalog.storageSite = "T1_US_FNAL"; + aCatalog.volume = "FNAL_dCache_EOS"; + aCatalog.protocol = "XRootD"; + tmp.push_back(aCatalog); + //catalog for testing chained "rules" + aCatalog.site = "T1_US_FNAL"; + aCatalog.subSite = "T1_US_FNAL"; + aCatalog.storageSite = "T1_US_FNAL"; + aCatalog.volume = "FNAL_dCache_EOS"; + aCatalog.protocol = "root"; + tmp.push_back(aCatalog); + + //create the services + return edm::ServiceToken(edm::ServiceRegistry::createContaining( + std::unique_ptr(std::make_unique(std::move(tmp))))); + } +} // namespace edmtest::catalog + +#endif From cff58ae800c392e12765b9fe6a34d09cbd2c57d9 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Thu, 19 Dec 2024 00:37:54 +0100 Subject: [PATCH 2/4] Remove InputFileCatalog::fileNames_ as seemingly unnecessary The fileNames_ was used only in init(), and before being modified in init() it was a direct copy of logicalFileNames_. There doesn't seem to be any real need to store it as a data member, and removing it allows to save ~100 MB memory per stream on an example MC production (using premixing) DIGI job whose configuration specified had ~500k pileup files. --- FWCore/Catalog/interface/InputFileCatalog.h | 3 +-- FWCore/Catalog/src/InputFileCatalog.cc | 23 ++++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/FWCore/Catalog/interface/InputFileCatalog.h b/FWCore/Catalog/interface/InputFileCatalog.h index 638cad95e7589..aecceed4881cb 100644 --- a/FWCore/Catalog/interface/InputFileCatalog.h +++ b/FWCore/Catalog/interface/InputFileCatalog.h @@ -19,7 +19,7 @@ namespace edm { class FileCatalogItem { public: - FileCatalogItem(std::vector const& pfns, std::string const& lfn) : pfns_(pfns), lfn_(lfn) {} + FileCatalogItem(std::vector pfns, std::string lfn) : pfns_(std::move(pfns)), lfn_(std::move(lfn)) {} std::string const& fileName(unsigned iCatalog) const { return pfns_[iCatalog]; } std::string const& logicalFileName() const { return lfn_; } @@ -54,7 +54,6 @@ namespace edm { bool useLFNasPFNifLFNnotFound, edm::CatalogType catType); std::vector logicalFileNames_; - std::vector fileNames_; std::vector fileCatalogItems_; edm::propagate_const> overrideFileLocator_; diff --git a/FWCore/Catalog/src/InputFileCatalog.cc b/FWCore/Catalog/src/InputFileCatalog.cc index c3eddb052f96a..27bbf39db7384 100644 --- a/FWCore/Catalog/src/InputFileCatalog.cc +++ b/FWCore/Catalog/src/InputFileCatalog.cc @@ -21,7 +21,7 @@ namespace edm { std::string const& override, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType) - : logicalFileNames_(fileNames), fileNames_(fileNames), fileCatalogItems_(), overrideFileLocator_() { + : logicalFileNames_(fileNames), fileCatalogItems_(), overrideFileLocator_() { init(override, useLFNasPFNifLFNnotFound, catType); } @@ -123,32 +123,31 @@ namespace edm { throw ex; } - for (iter it = fileNames_.begin(), lt = logicalFileNames_.begin(), itEnd = fileNames_.end(); it != itEnd; - ++it, ++lt) { - boost::trim(*it); + for (auto& lfn : logicalFileNames_) { + boost::trim(lfn); std::vector pfns; - if (it->empty()) { + if (lfn.empty()) { cms::Exception ex("FileCatalog"); ex << "An empty string specified in the fileNames parameter for input source"; ex.addContext("Calling edm::InputFileCatalog::init()"); throw ex; } - if (isPhysical(*it)) { - if (it->back() == ':') { + if (isPhysical(lfn)) { + if (lfn.back() == ':') { cms::Exception ex("FileCatalog"); ex << "An empty physical file name specified in the fileNames parameter for input source"; ex.addContext("Calling edm::InputFileCatalog::init()"); throw ex; } - pfns.push_back(*it); + pfns.push_back(lfn); // Clear the LFN. - lt->clear(); + lfn.clear(); } else { - boost::trim(*lt); - findFile(*lt, pfns, useLFNasPFNifLFNnotFound, catType); + findFile(lfn, pfns, useLFNasPFNifLFNnotFound, catType); } + lfn.shrink_to_fit(); // try to release memory - fileCatalogItems_.push_back(FileCatalogItem(pfns, *lt)); + fileCatalogItems_.emplace_back(std::move(pfns), lfn); } } From 3b2822e6bb456ac59ee5d79b9d2b89a1e933d827 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Thu, 19 Dec 2024 01:03:02 +0100 Subject: [PATCH 3/4] Remove InputFileCatalog::logicalFileNames as unnecessary The logicalFileNames() was not used, so removing the member allows avoiding one copy of the LFNs. --- FWCore/Catalog/interface/InputFileCatalog.h | 7 +++--- FWCore/Catalog/src/InputFileCatalog.cc | 11 +++++---- FWCore/Catalog/test/InputFileCatalog_t.cpp | 24 ------------------- FWCore/Sources/interface/FromFiles.h | 1 - .../interface/ProducerSourceFromFiles.h | 1 - .../interface/RawInputSourceFromFiles.h | 1 - 6 files changed, 10 insertions(+), 35 deletions(-) diff --git a/FWCore/Catalog/interface/InputFileCatalog.h b/FWCore/Catalog/interface/InputFileCatalog.h index aecceed4881cb..2bdf2c39cd4da 100644 --- a/FWCore/Catalog/interface/InputFileCatalog.h +++ b/FWCore/Catalog/interface/InputFileCatalog.h @@ -42,18 +42,19 @@ namespace edm { ~InputFileCatalog(); std::vector const& fileCatalogItems() const { return fileCatalogItems_; } - std::vector const& logicalFileNames() const { return logicalFileNames_; } std::vector fileNames(unsigned iCatalog) const; bool empty() const { return fileCatalogItems_.empty(); } static bool isPhysical(std::string const& name) { return (name.empty() || name.find(':') != std::string::npos); } private: - void init(std::string const& override, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType); + void init(std::vector logicalFileNames, + std::string const& override, + bool useLFNasPFNifLFNnotFound, + edm::CatalogType catType); void findFile(std::string const& lfn, std::vector& pfns, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType); - std::vector logicalFileNames_; std::vector fileCatalogItems_; edm::propagate_const> overrideFileLocator_; diff --git a/FWCore/Catalog/src/InputFileCatalog.cc b/FWCore/Catalog/src/InputFileCatalog.cc index 27bbf39db7384..2f85e71855d43 100644 --- a/FWCore/Catalog/src/InputFileCatalog.cc +++ b/FWCore/Catalog/src/InputFileCatalog.cc @@ -21,8 +21,8 @@ namespace edm { std::string const& override, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType) - : logicalFileNames_(fileNames), fileCatalogItems_(), overrideFileLocator_() { - init(override, useLFNasPFNifLFNnotFound, catType); + : fileCatalogItems_(), overrideFileLocator_() { + init(fileNames, override, useLFNasPFNifLFNnotFound, catType); } InputFileCatalog::~InputFileCatalog() {} @@ -36,7 +36,8 @@ namespace edm { return tmp; } - void InputFileCatalog::init(std::string const& inputOverride, + void InputFileCatalog::init(std::vector logicalFileNames, + std::string const& inputOverride, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType) { typedef std::vector::iterator iter; @@ -123,7 +124,7 @@ namespace edm { throw ex; } - for (auto& lfn : logicalFileNames_) { + for (auto& lfn : logicalFileNames) { boost::trim(lfn); std::vector pfns; if (lfn.empty()) { @@ -147,7 +148,7 @@ namespace edm { } lfn.shrink_to_fit(); // try to release memory - fileCatalogItems_.emplace_back(std::move(pfns), lfn); + fileCatalogItems_.emplace_back(std::move(pfns), std::move(lfn)); } } diff --git a/FWCore/Catalog/test/InputFileCatalog_t.cpp b/FWCore/Catalog/test/InputFileCatalog_t.cpp index 52edef9f9d3bc..571db9956fd44 100644 --- a/FWCore/Catalog/test/InputFileCatalog_t.cpp +++ b/FWCore/Catalog/test/InputFileCatalog_t.cpp @@ -25,14 +25,6 @@ TEST_CASE("InputFileCatalog with Rucio data catalog", "[FWCore/Catalog]") { edm::InputFileCatalog catalog(std::vector{"/store/foo/bar", " file:/foo/bar", "root://foobar "}, ""); - SECTION("logicalFileNames") { - auto const& lfns = catalog.logicalFileNames(); - REQUIRE(lfns.size() == 3); - CHECK(lfns[0] == "/store/foo/bar"); - CHECK(lfns[1] == ""); // was PFN - CHECK(lfns[2] == ""); // was PFN - } - SECTION("fileNames") { SECTION("Catalog 0") { auto const names = catalog.fileNames(0); @@ -76,14 +68,6 @@ TEST_CASE("InputFileCatalog with Rucio data catalog", "[FWCore/Catalog]") { edm::InputFileCatalog catalog(std::vector{"/store/foo/bar", " file:/foo/bar", "root://foobar "}, "T1_US_FNAL,,T1_US_FNAL,FNAL_dCache_EOS,XRootD"); - SECTION("logicalFileNames") { - auto const& lfns = catalog.logicalFileNames(); - REQUIRE(lfns.size() == 3); - CHECK(lfns[0] == "/store/foo/bar"); - CHECK(lfns[1] == ""); // was PFN - CHECK(lfns[2] == ""); // was PFN - } - SECTION("fileNames") { auto const names = catalog.fileNames(0); REQUIRE(names.size() == 3); @@ -116,14 +100,6 @@ TEST_CASE("InputFileCatalog with Rucio data catalog", "[FWCore/Catalog]") { edm::InputFileCatalog catalog( std::vector{"/store/foo/bar", "/tmp/foo/bar", "root://foobar "}, "", true); - SECTION("logicalFileNames") { - auto const& lfns = catalog.logicalFileNames(); - REQUIRE(lfns.size() == 3); - CHECK(lfns[0] == "/store/foo/bar"); - CHECK(lfns[1] == "/tmp/foo/bar"); - CHECK(lfns[2] == ""); // was PFN - } - SECTION("fileNames") { SECTION("Catalog 0") { auto const names = catalog.fileNames(0); diff --git a/FWCore/Sources/interface/FromFiles.h b/FWCore/Sources/interface/FromFiles.h index c54af177b4406..f756e914ce53c 100644 --- a/FWCore/Sources/interface/FromFiles.h +++ b/FWCore/Sources/interface/FromFiles.h @@ -18,7 +18,6 @@ namespace edm { FromFiles(ParameterSet const& pset); ~FromFiles(); - std::vector const& logicalFileNames() const { return catalog_.logicalFileNames(); } std::vector fileNames(unsigned iCatalog) const { return catalog_.fileNames(iCatalog); } InputFileCatalog& catalog() { return catalog_; } diff --git a/FWCore/Sources/interface/ProducerSourceFromFiles.h b/FWCore/Sources/interface/ProducerSourceFromFiles.h index 9f70f5b7a1a30..49150c030d25f 100644 --- a/FWCore/Sources/interface/ProducerSourceFromFiles.h +++ b/FWCore/Sources/interface/ProducerSourceFromFiles.h @@ -21,7 +21,6 @@ namespace edm { using FromFiles::catalog; using FromFiles::fileNames; - using FromFiles::logicalFileNames; static void fillDescription(ParameterSetDescription& desc); diff --git a/FWCore/Sources/interface/RawInputSourceFromFiles.h b/FWCore/Sources/interface/RawInputSourceFromFiles.h index 8b0f7e3f73c0e..843e562d4232d 100644 --- a/FWCore/Sources/interface/RawInputSourceFromFiles.h +++ b/FWCore/Sources/interface/RawInputSourceFromFiles.h @@ -20,7 +20,6 @@ namespace edm { ~RawInputSourceFromFiles() override; using FromFiles::catalog; - using FromFiles::logicalFileNames; static void fillDescription(ParameterSetDescription& desc); From 5744eaa1862738f0934b26a6a209c181376faaa7 Mon Sep 17 00:00:00 2001 From: Matti Kortelainen Date: Thu, 19 Dec 2024 15:05:39 +0100 Subject: [PATCH 4/4] Take fileNames by value in InputFileCatalog constructor The constructor makes a copy of the fileNames anyway. For the cases where InputFileCatalog is constructed with temporary or moved vector, this change allows to avoid copying that vector. The Sources tend to pass in directly a temporary vector from the ParameterSet, so there could be a visible reduction in memory churn. --- FWCore/Catalog/interface/InputFileCatalog.h | 2 +- FWCore/Catalog/src/InputFileCatalog.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/FWCore/Catalog/interface/InputFileCatalog.h b/FWCore/Catalog/interface/InputFileCatalog.h index 2bdf2c39cd4da..ec99b1eb218be 100644 --- a/FWCore/Catalog/interface/InputFileCatalog.h +++ b/FWCore/Catalog/interface/InputFileCatalog.h @@ -33,7 +33,7 @@ namespace edm { class InputFileCatalog { public: - InputFileCatalog(std::vector const& fileNames, + InputFileCatalog(std::vector fileNames, std::string const& override, bool useLFNasPFNifLFNnotFound = false, //switching between two catalog types diff --git a/FWCore/Catalog/src/InputFileCatalog.cc b/FWCore/Catalog/src/InputFileCatalog.cc index 2f85e71855d43..fab27a17c1059 100644 --- a/FWCore/Catalog/src/InputFileCatalog.cc +++ b/FWCore/Catalog/src/InputFileCatalog.cc @@ -17,12 +17,12 @@ namespace edm { - InputFileCatalog::InputFileCatalog(std::vector const& fileNames, + InputFileCatalog::InputFileCatalog(std::vector fileNames, std::string const& override, bool useLFNasPFNifLFNnotFound, edm::CatalogType catType) : fileCatalogItems_(), overrideFileLocator_() { - init(fileNames, override, useLFNasPFNifLFNnotFound, catType); + init(std::move(fileNames), override, useLFNasPFNifLFNnotFound, catType); } InputFileCatalog::~InputFileCatalog() {}