From 2a5d6bf171a59a170f70f0a24407976731a39a2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mario=20Kr=C3=BCger?= Date: Tue, 15 Dec 2020 16:25:18 +0100 Subject: [PATCH] DPL Analysis: make sure each name hash in histogram registry is unique --- .../include/Framework/HistogramRegistry.h | 3 ++ Framework/Core/src/HistogramRegistry.cxx | 31 ++++++++++++++----- .../Core/test/test_HistogramRegistry.cxx | 16 ++++++++++ 3 files changed, 42 insertions(+), 8 deletions(-) diff --git a/Framework/Core/include/Framework/HistogramRegistry.h b/Framework/Core/include/Framework/HistogramRegistry.h index 549313ee345cd..d201c5d6f6d5f 100644 --- a/Framework/Core/include/Framework/HistogramRegistry.h +++ b/Framework/Core/include/Framework/HistogramRegistry.h @@ -615,6 +615,7 @@ class HistogramRegistry template void insertClone(const HistName& histName, const std::shared_ptr& originalHist) { + validateHash(histName.hash, histName.str); for (auto i = 0u; i < MAX_REGISTRY_SIZE; ++i) { TObject* rawPtr = nullptr; std::visit([&](const auto& sharedPtr) { rawPtr = sharedPtr.get(); }, mRegistryValue[imask(histName.idx + i)]); @@ -629,6 +630,8 @@ class HistogramRegistry LOGF(FATAL, "Internal array of HistogramRegistry %s is full.", mName); } + void validateHash(const uint32_t hash, const char* name); + // helper function to find the histogram position in the registry template uint32_t getHistIndex(const T& histName) diff --git a/Framework/Core/src/HistogramRegistry.cxx b/Framework/Core/src/HistogramRegistry.cxx index 7e50b299f0c65..bb19012367fe3 100644 --- a/Framework/Core/src/HistogramRegistry.cxx +++ b/Framework/Core/src/HistogramRegistry.cxx @@ -37,21 +37,33 @@ const std::map> HistFacto // create histogram from specification and insert it into the registry void HistogramRegistry::insert(const HistogramSpec& histSpec) { - const uint32_t i = imask(histSpec.hash); - for (auto j = 0u; j < MAX_REGISTRY_SIZE; ++j) { + validateHash(histSpec.hash, histSpec.name.data()); + const uint32_t idx = imask(histSpec.hash); + for (auto i = 0u; i < MAX_REGISTRY_SIZE; ++i) { TObject* rawPtr = nullptr; - std::visit([&](const auto& sharedPtr) { rawPtr = sharedPtr.get(); }, mRegistryValue[imask(j + i)]); + std::visit([&](const auto& sharedPtr) { rawPtr = sharedPtr.get(); }, mRegistryValue[imask(idx + i)]); if (!rawPtr) { registerName(histSpec.name); - mRegistryKey[imask(j + i)] = histSpec.hash; - mRegistryValue[imask(j + i)] = HistFactory::createHistVariant(histSpec); - lookup += j; + mRegistryKey[imask(idx + i)] = histSpec.hash; + mRegistryValue[imask(idx + i)] = HistFactory::createHistVariant(histSpec); + lookup += i; return; } } LOGF(FATAL, R"(Internal array of HistogramRegistry "%s" is full.)", mName); } +void HistogramRegistry::validateHash(const uint32_t hash, const char* name) +{ + auto it = std::find(mRegistryKey.begin(), mRegistryKey.end(), hash); + if (it != mRegistryKey.end()) { + auto idx = it - mRegistryKey.begin(); + std::string collidingName{}; + std::visit([&](const auto& hist) { collidingName = hist->GetName(); }, mRegistryValue[idx]); + LOGF(FATAL, R"(Hash collision in HistogramRegistry "%s"! Please rename histogram "%s" or "%s".)", mName, name, collidingName); + } +} + void HistogramRegistry::add(const HistogramSpec& histSpec) { insert(histSpec); @@ -201,6 +213,9 @@ void HistogramRegistry::print(bool showAxisDetails) } LOGF(INFO, "%s", std::string(titleString.size(), '='), titleString); LOGF(INFO, "Total: %d histograms, ca. %s", nHistos, totalSizeInfo); + if (lookup) { + LOGF(INFO, "Due to index collisions, histograms were shifted by %d registry slots in total.", lookup); + } LOGF(INFO, "%s", std::string(titleString.size(), '='), titleString); LOGF(INFO, ""); } @@ -211,9 +226,9 @@ TList* HistogramRegistry::operator*() TList* list = new TList(); list->SetName(mName.data()); - for (auto j = 0u; j < MAX_REGISTRY_SIZE; ++j) { + for (auto i = 0u; i < MAX_REGISTRY_SIZE; ++i) { TNamed* rawPtr = nullptr; - std::visit([&](const auto& sharedPtr) { rawPtr = (TNamed*)sharedPtr.get(); }, mRegistryValue[j]); + std::visit([&](const auto& sharedPtr) { rawPtr = (TNamed*)sharedPtr.get(); }, mRegistryValue[i]); if (rawPtr) { std::deque path = splitPath(rawPtr->GetName()); std::string name = path.back(); diff --git a/Framework/Core/test/test_HistogramRegistry.cxx b/Framework/Core/test/test_HistogramRegistry.cxx index 0cfa8d50b03e4..d59587c444ad3 100644 --- a/Framework/Core/test/test_HistogramRegistry.cxx +++ b/Framework/Core/test/test_HistogramRegistry.cxx @@ -13,6 +13,7 @@ #include "Framework/HistogramRegistry.h" #include +#include using namespace o2; using namespace o2::framework; @@ -55,6 +56,21 @@ BOOST_AUTO_TEST_CASE(HistogramRegistryLookup) auto r = foo(); auto histo2 = r.get(HIST("histo")).get(); BOOST_REQUIRE_EQUAL(histo2->GetNbinsX(), 100); + + registry.print(); + + // check that registry behaves correctly when two different names have equal hash: + /* + auto str1 = "Maria has nine red beds."; + auto str2 = "Steven has fifteen white tables."; + BOOST_REQUIRE_EQUAL(compile_time_hash(str1), compile_time_hash(str2)); + try { + registry.add(str1, "", kTH1F, {{20, 0.0f, 10.01f}}); + registry.add(str2, "", kTH1F, {{20, 0.0f, 10.01f}}); + } catch (...) { + std::cout << "Hash collision was detected correctly!" << std::endl; + } + */ } BOOST_AUTO_TEST_CASE(HistogramRegistryExpressionFill)