Modernize RPCTechnicalTrigger #25267

Dr15Jones · 2018-11-16T21:48:46Z

Improved memory management via std::unique_ptr or holding objects by value. This fixed several memory leaks. There are no remaining bare new or delete in this code.
Removed unnecessary member data.
Improved const correctness.
Removed unnecessary virtual destructors.
Changed to range based for loops.
Used more efficient patterns for containers.
Converted RPCTechnicalTrigger to a stream module.

Consolidated the setup for geometry description. If possible, converted to a constexpr.

Have CreateObject return a std::unique_ptr. Unregister never needed to call delete since new objects were created each call.

-Objects are returned by std::unique_ptr -All instances share an immutable factory

-Improved memory management via std::unique_ptr or holding objects by value. -Removed unnecessary member data. -Improved const correctness. -Removed unnecessary virtual destructors. -Changed to range based for loops. -Used more efficient patterns for containers.

cmsbuild · 2018-11-16T21:49:06Z

The code-checks are being triggered in jenkins.

Dr15Jones · 2018-11-16T21:49:32Z

I ran valgrind on step2 of workflow 1.0 which calls RPCTechnicalTrigger. No problem were seen.

cmsbuild · 2018-11-16T21:51:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25267/7292

cmsbuild · 2018-11-16T21:51:25Z

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

L1Trigger/RPCTechnicalTrigger

@nsmith-, @rekovic, @cmsbuild, @thomreis can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2018-11-16T21:50:43Z

L1Trigger/RPCTechnicalTrigger/interface/LogicFactory.h

-    typename std::map<Identifier, LogicCreator>::const_iterator itr;
-    itr = m_associations.find(id);
-    if( itr != m_associations.end() ) {
-      delete ( itr->second )() ;


This was not needed since the call to the function always returned a new object anyway.

Dr15Jones · 2018-11-16T21:52:30Z

L1Trigger/RPCTechnicalTrigger/interface/LogicTool.h

-
-private:
-
-  typename std::vector<std::string> m_logkeys;


m_logkeys was only being used to call m_rbclogic.Unregister() which was not necessary.

Dr15Jones · 2018-11-16T21:52:44Z

L1Trigger/RPCTechnicalTrigger/interface/LogicTool.h

-
-  typename std::vector<std::string> m_logkeys;
+  static RBCLogicType const& rbclogic() {
+    static const RBCLogicType s_rbclogic = initialise();


This is thread safe.

Dr15Jones · 2018-11-16T21:54:44Z

L1Trigger/RPCTechnicalTrigger/interface/RBCConfiguration.h

  const RBCBoardSpecs * m_rbcboardspecs;
+  std::unique_ptr<RBCLogicUnit>  m_rbclogic;


The class now properly manages its own member data rather than having classes which inherit from it try to manage the memory.

Dr15Jones · 2018-11-16T21:55:07Z

L1Trigger/RPCTechnicalTrigger/interface/RBCEmulator.h

@@ -32,8 +33,6 @@ class RBCEmulator {

  RBCEmulator( const char * , const char * , int, int *); 

-  virtual ~RBCEmulator( ); ///< Destructor


No classes inherit from this one.

Dr15Jones · 2018-11-16T22:24:34Z

L1Trigger/RPCTechnicalTrigger/src/RBCProcessRPCDigis.cc

    isAoB = 1;
  } 

  for ( pos = 0; pos < 6; ++pos ) {
-    if (this->m_block->m_sec1[pos] == sector || this->m_block->m_sec2[pos] == sector )


this->m_block is only being used as a hidden argument to the function. I made it explicit.

Dr15Jones · 2018-11-16T22:25:25Z

L1Trigger/RPCTechnicalTrigger/src/RBCProcessRPCDigis.cc

@@ -363,7 +272,7 @@ void RBCProcessRPCDigis::setDigiAt( int sector, int digipos )
 void RBCProcessRPCDigis::setInputBit( std::bitset<15> & signals , int digipos ) 
 {

-  int bitpos = m_layermap[digipos];
+  int bitpos = s_layermap.at(digipos);


This is a functional differerence. If an out of range digipos is used, this code will now throw. The old code just added a new entry.

Dr15Jones · 2018-11-16T22:27:56Z

L1Trigger/RPCTechnicalTrigger/src/RBCProcessRPCSimDigis.cc

@@ -344,7 +267,7 @@ void RBCProcessRPCSimDigis::setDigiAt( int sector, int digipos )
 void RBCProcessRPCSimDigis::setInputBit( std::bitset<15> & signals , int digipos ) 
 {

-  int bitpos = m_layermap[digipos];
+  int bitpos = s_layermap.at(digipos);


This is a functional difference, the code will now throw if digipos is not in the container.

Dr15Jones · 2018-11-16T22:33:00Z

L1Trigger/RPCTechnicalTrigger/src/RPCTechnicalTrigger.cc

  bool topRight(false);
  bool botLeft(false);

-  int indxW1 = m_WheelTtu[wheel1];
-  int indxW2 = m_WheelTtu[wheel2];
+  int indxW1 = wheelTtu[wheel1+kWheelOffset];


Previously a map was being used. But the keys were just {-2,-1,0,1,2}. So a std::vector with an offset can do the job better.

Dr15Jones · 2018-11-16T22:34:37Z

L1Trigger/RPCTechnicalTrigger/src/TTUBasicConfig.cc

-
-  m_ttuboardspecs = ttuspecs;
-
-  m_ttulogic      = new TTULogicUnit();


This is a member data from the base class.

Dr15Jones · 2018-11-16T22:40:40Z

please test

cmsbuild · 2018-11-16T22:40:59Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31706/console Started: 2018/11/17 01:12

Dr15Jones · 2018-11-16T22:50:59Z

Running valgrind with memory leaks checking on, I do not see any memory leaks coming from this package with my changes.

cmsbuild · 2018-11-17T00:16:36Z

-1

Tested at: f149ded

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25267/31706/summary.html

I found follow errors while testing this PR

Failed tests: ClangBuild

Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 16 COMPILER='llvm compile'

See details on the summary page.

cmsbuild · 2018-11-17T00:16:38Z

Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped)

cmsbuild · 2018-11-17T01:29:44Z

The code-checks are being triggered in jenkins.

cmsbuild · 2018-11-17T01:31:53Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25267/7293

cmsbuild · 2018-11-17T01:32:11Z

Pull request #25267 was updated. @nsmith-, @rekovic, @cmsbuild, @thomreis can you please check and sign again.

Dr15Jones · 2018-11-17T01:38:46Z

please test

cmsbuild · 2018-11-17T01:39:04Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31708/console Started: 2018/11/17 04:46

cmsbuild · 2018-11-17T06:06:32Z

+1
Tested at: ee6cc65
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25267/31708/summary.html

cmsbuild · 2018-11-17T06:06:35Z

Comparison job queued.

cmsbuild · 2018-11-17T07:53:04Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-25267/31708/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 32
DQMHistoTests: Total histograms compared: 3013311
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3013102
DQMHistoTests: Total skipped: 197
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
Checked 134 log files, 14 edm output root files, 32 DQM output files

fabiocos · 2018-11-18T17:34:20Z

DQM differences look unrelated to this PR

rekovic · 2018-11-20T06:34:05Z

+1

cmsbuild · 2018-11-20T06:34:25Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2018-11-20T09:10:44Z

+1

Dr15Jones added 5 commits November 16, 2018 14:48

Provide central description of geometry

5856619

Consolidated the setup for geometry description. If possible, converted to a constexpr.

Explicitly show that object ownership is transfered

d616268

Have CreateObject return a std::unique_ptr. Unregister never needed to call delete since new objects were created each call.

Member memory handling for LogicTool

378bb2e

-Objects are returned by std::unique_ptr -All instances share an immutable factory

Change RPCTechnicalTrigger to a stream module

f149ded

cmsbuild added this to the CMSSW_10_4_X milestone Nov 16, 2018

cmsbuild added code-checks-pending comparison-pending l1-pending orp-pending pending-signatures tests-pending labels Nov 16, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 16, 2018

Dr15Jones commented Nov 16, 2018

View reviewed changes

cmsbuild added tests-started and removed tests-pending labels Nov 16, 2018

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Nov 17, 2018

cmsbuild added comparison-pending tests-pending labels Nov 17, 2018

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 17, 2018

cmsbuild added tests-started and removed tests-pending labels Nov 17, 2018

cmsbuild added tests-approved and removed tests-started labels Nov 17, 2018

cmsbuild added comparison-available and removed comparison-pending labels Nov 17, 2018

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Nov 20, 2018

cmsbuild added orp-approved and removed orp-pending labels Nov 20, 2018

cmsbuild merged commit 445a255 into cms-sw:master Nov 20, 2018

Dr15Jones deleted the modernizeRPCTechnicalTrigger branch November 22, 2018 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize RPCTechnicalTrigger #25267

Modernize RPCTechnicalTrigger #25267

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones Nov 16, 2018

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018 •

edited

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

Dr15Jones commented Nov 17, 2018

cmsbuild commented Nov 17, 2018 •

edited

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

fabiocos commented Nov 18, 2018

rekovic commented Nov 20, 2018

cmsbuild commented Nov 20, 2018

fabiocos commented Nov 20, 2018

		const RBCBoardSpecs * m_rbcboardspecs;
		std::unique_ptr<RBCLogicUnit> m_rbclogic;

		@@ -32,8 +33,6 @@ class RBCEmulator {

		RBCEmulator( const char * , const char * , int, int *);

		virtual ~RBCEmulator( ); ///< Destructor

Modernize RPCTechnicalTrigger #25267

Modernize RPCTechnicalTrigger #25267

Conversation

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

cmsbuild commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 16, 2018 • edited

Dr15Jones commented Nov 16, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

Dr15Jones commented Nov 17, 2018

cmsbuild commented Nov 17, 2018 • edited

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

cmsbuild commented Nov 17, 2018

fabiocos commented Nov 18, 2018

rekovic commented Nov 20, 2018

cmsbuild commented Nov 20, 2018

fabiocos commented Nov 20, 2018

cmsbuild commented Nov 16, 2018 •

edited

cmsbuild commented Nov 17, 2018 •

edited