Merge pull request #9267 from dmitrijus/clonewars_74x

Do less cloning of TH1 objects in the DQMStore (backport to 74x)
cms-sw · Jun 16, 2015 · 1c0ad37 · 1c0ad37
2 parents c531ec5 + 79c210d
commit 1c0ad37
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 34 deletions.
diff --git a/DQMServices/Core/interface/MonitorElement.h b/DQMServices/Core/interface/MonitorElement.h
@@ -30,6 +30,9 @@
 
 class QCriterion;
 
+// tag for a special constructor, see below
+struct MonitorElementNoCloneTag {};
+
 /** The base class for all MonitorElements (ME) */
 class MonitorElement
 {
@@ -86,8 +89,11 @@ class MonitorElement
                  uint32_t run = 0,
                  uint32_t streamId = 0,
                  uint32_t moduleId = 0);
+  MonitorElement(const MonitorElement &, MonitorElementNoCloneTag);
   MonitorElement(const MonitorElement &);
-  MonitorElement &operator=(const MonitorElement &);
+  MonitorElement(MonitorElement &&);
+  MonitorElement &operator=(const MonitorElement &) = delete;
+  MonitorElement &operator=(MonitorElement &&) = delete;
   ~MonitorElement(void);
 
   /// Compare monitor elements, for ordering in sets.

diff --git a/DQMServices/Core/src/DQMStore.cc b/DQMServices/Core/src/DQMStore.cc
@@ -369,8 +369,11 @@ void DQMStore::mergeAndResetMEsRunSummaryCache(uint32_t run,
       continue;
     }
 
-    MonitorElement global_me(*i);
+    // don't call the copy constructor
+    // we are just searching for a global histogram - a copy is not necessary
+    MonitorElement global_me(*i, MonitorElementNoCloneTag());
     global_me.globalize();
+
     // Since this accesses the data, the operation must be
     // be locked.
     std::lock_guard<std::mutex> guard(book_mutex_);
@@ -396,7 +399,11 @@ void DQMStore::mergeAndResetMEsRunSummaryCache(uint32_t run,
       if (verbose_ > 1)
         std::cout << "No global Object found. " << std::endl;
       std::pair<std::set<MonitorElement>::const_iterator, bool> gme;
-      gme = data_.insert(global_me);
+
+      // this makes an actual and a single copy with Clone()'ed th1
+      MonitorElement actual_global_me(*i);
+      actual_global_me.globalize();
+      gme = data_.insert(std::move(actual_global_me));
       assert(gme.second);
     }
     // TODO(rovere): eventually reset the local object and mark it as reusable??
@@ -430,7 +437,7 @@ void DQMStore::mergeAndResetMEsLuminositySummaryCache(uint32_t run,
       continue;
     }
 
-    MonitorElement global_me(*i);
+    MonitorElement global_me(*i, MonitorElementNoCloneTag());
     global_me.globalize();
     global_me.setLumi(lumi);
     // Since this accesses the data, the operation must be
@@ -458,7 +465,12 @@ void DQMStore::mergeAndResetMEsLuminositySummaryCache(uint32_t run,
       if (verbose_ > 1)
         std::cout << "No global Object found. " << std::endl;
       std::pair<std::set<MonitorElement>::const_iterator, bool> gme;
-      gme = data_.insert(global_me);
+
+      // this makes an actual and a single copy with Clone()'ed th1
+      MonitorElement actual_global_me(*i);
+      actual_global_me.globalize();
+      actual_global_me.setLumi(lumi);
+      gme = data_.insert(std::move(actual_global_me));
       assert(gme.second);
     }
     // make the ME reusable for the next LS
@@ -817,7 +829,7 @@ DQMStore::book(const std::string &dir, const std::string &name,
     // Create and initialise core object.
     assert(dirs_.count(dir));
     MonitorElement proto(&*dirs_.find(dir), name, run_, streamId_, moduleId_);
-    me = const_cast<MonitorElement &>(*data_.insert(proto).first)
+    me = const_cast<MonitorElement &>(*data_.insert(std::move(proto)).first)
       .initialise((MonitorElement::Kind)kind, h);
 
     // Initialise quality test information.
@@ -874,7 +886,7 @@ DQMStore::book(const std::string &dir,
     // Create it and return for initialisation.
     assert(dirs_.count(dir));
     MonitorElement proto(&*dirs_.find(dir), name, run_, streamId_, moduleId_);
-    return &const_cast<MonitorElement &>(*data_.insert(proto).first);
+    return &const_cast<MonitorElement &>(*data_.insert(std::move(proto)).first);
   }
 }
 

diff --git a/DQMServices/Core/src/MonitorElement.cc b/DQMServices/Core/src/MonitorElement.cc
@@ -183,44 +183,34 @@ MonitorElement::MonitorElement(const std::string *path,
   scalar_.real = 0;
 }
 
-MonitorElement::MonitorElement(const MonitorElement &x)
+MonitorElement::MonitorElement(const MonitorElement &x, MonitorElementNoCloneTag)
   : data_(x.data_),
     scalar_(x.scalar_),
-    object_(x.object_),
+    object_(nullptr),
     reference_(x.reference_),
-    refvalue_(x.refvalue_),
+    refvalue_(nullptr),
     qreports_(x.qreports_)
 {
-  if (object_)
-    object_ = static_cast<TH1 *>(object_->Clone());
-
-  if (refvalue_)
-    refvalue_ = static_cast<TH1 *>(refvalue_->Clone());
 }
 
-MonitorElement &
-MonitorElement::operator=(const MonitorElement &x)
+MonitorElement::MonitorElement(const MonitorElement &x)
+  : MonitorElement::MonitorElement(x, MonitorElementNoCloneTag())
 {
-  if (this != &x)
-  {
-    delete object_;
-    delete refvalue_;
-
-    data_ = x.data_;
-    scalar_ = x.scalar_;
-    object_ = x.object_;
-    reference_ = x.reference_;
-    refvalue_ = x.refvalue_;
-    qreports_ = x.qreports_;
+  if (x.object_)
+    object_ = static_cast<TH1 *>(x.object_->Clone());
 
-    if (object_)
-      object_ = static_cast<TH1 *>(object_->Clone());
+  if (x.refvalue_)
+    refvalue_ = static_cast<TH1 *>(x.refvalue_->Clone());
+}
 
-    if (refvalue_)
-      refvalue_ = static_cast<TH1 *>(refvalue_->Clone());
-  }
+MonitorElement::MonitorElement(MonitorElement &&o)
+  : MonitorElement::MonitorElement(o, MonitorElementNoCloneTag())
+{
+  object_ = o.object_;
+  refvalue_ = o.refvalue_;
 
-  return *this;
+  o.object_ = nullptr;
+  o.refvalue_ = nullptr;
 }
 
 MonitorElement::~MonitorElement(void)