DPL Analysis: make sure each name hash in histogram registry is unique (#5068)

Author: mario-krueger

Framework/Core/include/Framework/HistogramRegistry.h

@@ -615,6 +615,7 @@ class HistogramRegistry
   template <typename T>
   void insertClone(const HistName& histName, const std::shared_ptr<T>& 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 <typename T>
   uint32_t getHistIndex(const T& histName)

Framework/Core/src/HistogramRegistry.cxx

@@ -37,21 +37,33 @@ const std::map<HistType, std::function<HistPtr(const HistogramSpec&)>> 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<std::string> path = splitPath(rawPtr->GetName());
       std::string name = path.back();

Framework/Core/test/test_HistogramRegistry.cxx

@@ -13,6 +13,7 @@
 
 #include "Framework/HistogramRegistry.h"
 #include <boost/test/unit_test.hpp>
+#include <iostream>
 
 using namespace o2;
 using namespace o2::framework;
@@ -55,6 +56,21 @@ BOOST_AUTO_TEST_CASE(HistogramRegistryLookup)
   auto r = foo();
   auto histo2 = r.get<TH1>(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)