more code review comments

cms-sw · Mar 11, 2020 · c67d250 · c67d250
1 parent f03f117
commit c67d250
Show file tree

Hide file tree

Showing 11 changed files with 30 additions and 71 deletions.
diff --git a/HeterogeneousCore/SonicCore/interface/SonicClientAsync.h b/HeterogeneousCore/SonicCore/interface/SonicClientAsync.h
@@ -9,8 +9,6 @@
 template <typename InputT, typename OutputT = InputT>
 class SonicClientAsync : public SonicClientBase, public SonicClientTypes<InputT, OutputT> {
 public:
-  virtual ~SonicClientAsync() {}
-
   //main operation
   void dispatch(edm::WaitingTaskWithArenaHolder holder) override final {
     holder_ = std::move(holder);

diff --git a/HeterogeneousCore/SonicCore/interface/SonicClientBase.h b/HeterogeneousCore/SonicCore/interface/SonicClientBase.h
@@ -14,6 +14,7 @@ class SonicClientBase {
   virtual ~SonicClientBase() = default;
 
   void setDebugName(const std::string& debugName) { debugName_ = debugName; }
+  const std::string& debugName() const { return debugName_; }
 
   //main operation
   virtual void dispatch(edm::WaitingTaskWithArenaHolder holder) = 0;
@@ -25,11 +26,10 @@ class SonicClientBase {
     if (debugName_.empty())
       return;
     t0_ = std::chrono::high_resolution_clock::now();
-    setTime_ = true;
   }
 
   void finish(std::exception_ptr eptr = std::exception_ptr{}) {
-    if (setTime_) {
+    if (!debugName_.empty()) {
       auto t1 = std::chrono::high_resolution_clock::now();
       edm::LogInfo(debugName_) << "Client time: "
                                << std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0_).count();
@@ -43,7 +43,6 @@ class SonicClientBase {
   //for logging/debugging
   std::string debugName_;
   std::chrono::time_point<std::chrono::high_resolution_clock> t0_;
-  bool setTime_ = false;
 };
 
 #endif
diff --git a/HeterogeneousCore/SonicCore/interface/SonicClientPseudoAsync.h b/HeterogeneousCore/SonicCore/interface/SonicClientPseudoAsync.h
@@ -21,15 +21,15 @@ class SonicClientPseudoAsync : public SonicClientBase, public SonicClientTypes<I
     thread_ = std::make_unique<std::thread>([this]() { waitForNext(); });
   }
   //destructor
-  virtual ~SonicClientPseudoAsync() {
-    {
-      std::lock_guard<std::mutex> guard(mutex_);
-      stop_ = true;
-    }
+  ~SonicClientPseudoAsync() override {
+    stop_ = true;
     cond_.notify_one();
     if (thread_) {
-      thread_->join();
-      thread_.reset();
+      try {
+        thread_->join();
+        thread_.reset();
+      } catch (...) {
+      }
     }
   }
   //accessor
@@ -46,7 +46,7 @@ class SonicClientPseudoAsync : public SonicClientBase, public SonicClientTypes<I
     cond_.notify_one();
   }
 
-protected:
+private:
   void waitForNext() {
     while (true) {
       //wait for condition

diff --git a/HeterogeneousCore/SonicCore/interface/SonicClientSync.h b/HeterogeneousCore/SonicCore/interface/SonicClientSync.h
@@ -11,8 +11,6 @@
 template <typename InputT, typename OutputT = InputT>
 class SonicClientSync : public SonicClientBase, public SonicClientTypes<InputT, OutputT> {
 public:
-  virtual ~SonicClientSync() {}
-
   //main operation
   void dispatch(edm::WaitingTaskWithArenaHolder holder) override final {
     holder_ = std::move(holder);

diff --git a/HeterogeneousCore/SonicCore/interface/SonicClientTypes.h b/HeterogeneousCore/SonicCore/interface/SonicClientTypes.h
@@ -1,8 +1,6 @@
 #ifndef HeterogeneousCore_SonicCore_SonicClientTypes
 #define HeterogeneousCore_SonicCore_SonicClientTypes
 
-#include "FWCore/ParameterSet/interface/ParameterSet.h"
-
 //this base class exists to limit the impact of dependent scope in derived classes
 template <typename InputT, typename OutputT = InputT>
 class SonicClientTypes {

diff --git a/HeterogeneousCore/SonicCore/interface/SonicEDProducer.h b/HeterogeneousCore/SonicCore/interface/SonicEDProducer.h
@@ -32,9 +32,9 @@ class SonicEDProducer : public edm::stream::EDProducer<edm::ExternalWork, Capabi
     auto t0 = std::chrono::high_resolution_clock::now();
     acquire(iEvent, iSetup, client_.input());
     auto t1 = std::chrono::high_resolution_clock::now();
-    if (!debugName_.empty())
-      edm::LogInfo(debugName_) << "Load time: "
-                               << std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();
+    if (!client_.debugName().empty())
+      edm::LogInfo(client_.debugName()) << "Load time: "
+                                        << std::chrono::duration_cast<std::chrono::microseconds>(t1 - t0).count();
     client_.dispatch(holder);
   }
   virtual void acquire(edm::Event const& iEvent, edm::EventSetup const& iSetup, Input& iInput) = 0;
@@ -47,13 +47,9 @@ class SonicEDProducer : public edm::stream::EDProducer<edm::ExternalWork, Capabi
 
 protected:
   //for debugging
-  void setDebugName(const std::string& debugName) {
-    debugName_ = debugName;
-    client_.setDebugName(debugName);
-  }
+  void setDebugName(const std::string& debugName) { client_.setDebugName(debugName); }
   //members
   Client client_;
-  std::string debugName_;
 };
 
 #endif
diff --git a/HeterogeneousCore/SonicCore/test/BuildFile.xml b/HeterogeneousCore/SonicCore/test/BuildFile.xml
@@ -1,8 +1,5 @@
 <environment>
-  <bin file="TestIntegration.cpp" name="TestIntegrationSonic">
-    <flags TEST_RUNNER_ARGS=" /bin/bash HeterogeneousCore/SonicCore/test run_sonic_test.sh"/>
-    <use name="FWCore/Utilities"/>
-  </bin>
+  <test name="TestHeterogeneousCoreSonicCore" command="cmsRun ${LOCALTOP}/src/HeterogeneousCore/SonicCore/test/sonicTest_cfg.py"/>
   <library file="SonicDummyProducer.cc">
     <flags EDM_PLUGIN="1"/>
     <use name="HeterogeneousCore/SonicCore"/>

diff --git a/HeterogeneousCore/SonicCore/test/SonicDummyProducer.cc b/HeterogeneousCore/SonicCore/test/SonicDummyProducer.cc
@@ -6,7 +6,7 @@
 
 #include <memory>
 
-namespace edmtest {
+namespace sonictest {
   template <typename Client>
   class SonicDummyProducer : public SonicEDProducer<Client> {
   public:
@@ -15,50 +15,39 @@ namespace edmtest {
     using typename SonicEDProducer<Client>::Output;
     explicit SonicDummyProducer(edm::ParameterSet const& cfg)
         : SonicEDProducer<Client>(cfg), input_(cfg.getParameter<int>("input")) {
-      this->template produces<IntProduct>();
+      //for debugging
+      this->setDebugName("SonicDummyProducer");
+      putToken_ = this->template produces<edmtest::IntProduct>();
     }
 
     void acquire(edm::Event const& iEvent, edm::EventSetup const& iSetup, Input& iInput) override { iInput = input_; }
 
     void produce(edm::Event& iEvent, edm::EventSetup const& iSetup, Output const& iOutput) override {
-      iEvent.put(std::make_unique<IntProduct>(iOutput));
+      iEvent.emplace(putToken_, iOutput);
     }
 
-    //to ensure distinct cfi names - specialized below
-    static std::string getCfiName();
     static void fillDescriptions(edm::ConfigurationDescriptions& descriptions) {
       edm::ParameterSetDescription desc;
       Client::fillPSetDescription(desc);
       desc.add<int>("input");
-      descriptions.add(getCfiName(), desc);
+      //to ensure distinct cfi names
+      descriptions.addWithDefaultLabel(desc);
     }
 
   private:
     //members
     int input_;
+    edm::EDPutTokenT<edmtest::IntProduct> putToken_;
   };
 
   typedef SonicDummyProducer<DummyClientSync> SonicDummyProducerSync;
   typedef SonicDummyProducer<DummyClientPseudoAsync> SonicDummyProducerPseudoAsync;
   typedef SonicDummyProducer<DummyClientAsync> SonicDummyProducerAsync;
+}  // namespace sonictest
 
-  template <>
-  std::string SonicDummyProducerSync::getCfiName() {
-    return "SonicDummyProducerSync";
-  }
-  template <>
-  std::string SonicDummyProducerPseudoAsync::getCfiName() {
-    return "SonicDummyProducerPseudoAsync";
-  }
-  template <>
-  std::string SonicDummyProducerAsync::getCfiName() {
-    return "SonicDummyProducerAsync";
-  }
-}  // namespace edmtest
-
-using edmtest::SonicDummyProducerSync;
+using sonictest::SonicDummyProducerSync;
 DEFINE_FWK_MODULE(SonicDummyProducerSync);
-using edmtest::SonicDummyProducerPseudoAsync;
+using sonictest::SonicDummyProducerPseudoAsync;
 DEFINE_FWK_MODULE(SonicDummyProducerPseudoAsync);
-using edmtest::SonicDummyProducerAsync;
+using sonictest::SonicDummyProducerAsync;
 DEFINE_FWK_MODULE(SonicDummyProducerAsync);
diff --git a/HeterogeneousCore/SonicCore/test/TestIntegration.cpp b/HeterogeneousCore/SonicCore/test/TestIntegration.cpp
diff --git a/HeterogeneousCore/SonicCore/test/run_sonic_test.sh b/HeterogeneousCore/SonicCore/test/run_sonic_test.sh
diff --git a/HeterogeneousCore/SonicCore/test/sonicTest_cfg.py b/HeterogeneousCore/SonicCore/test/sonicTest_cfg.py
@@ -4,12 +4,10 @@
 
 process.source = cms.Source("EmptySource")
 
-process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(1))
+process.maxEvents.input = 1
 
-process.options = cms.untracked.PSet(
-    numberOfThreads = cms.untracked.uint32(2),
-    numberOfStreams = cms.untracked.uint32(0)
-)
+process.options.numberOfThreads = 2
+process.options.numberOfStreams = 0
 
 process.dummySync = cms.EDProducer("SonicDummyProducerSync",
     input = cms.int32(1),