Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Test clang-tidy --checks performance-inefficient-vector-operation #29930

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented May 21, 2020

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29930/15546

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

.clang-tidy
Alignment/OfflineValidation
Alignment/TrackerAlignment
CalibCalorimetry/HcalAlgos
CalibPPS/ESProducers
CalibTracker/SiStripAPVAnalysis
CaloOnlineTools/EcalTools
CondFormats/HcalObjects
CondFormats/SiPixelObjects
CondTools/Ecal
DQM/HcalCommon
DQMOffline/CalibTracker
DQMOffline/L1Trigger
DQMOffline/Trigger
DQMServices/Core
DataFormats/L1Trigger
DataFormats/PatCandidates
ElectroWeakAnalysis/ZMuMu
FastSimulation/CTPPSFastTrackingProducer
FastSimulation/CTPPSRecHitProducer
Fireworks/Core
Fireworks/ParticleFlow
Geometry/EcalMapping
Geometry/HGCalGeometry
HLTriggerOffline/SUSYBSM
L1Trigger/GlobalCaloTrigger
L1Trigger/L1TCalorimeter
L1Trigger/L1TGEM
L1Trigger/L1THGCal
L1TriggerConfig/L1TConfigProducers
L1TriggerConfig/RPCTriggerConfig
MuonAnalysis/MomentumScaleCalibration
PhysicsTools/Heppy
PhysicsTools/JetMCAlgos
RecoEcal/EgammaClusterAlgos
RecoEgamma/EgammaTools
RecoEgamma/ElectronIdentification
RecoEgamma/PhotonIdentification
RecoHI/HiJetAlgos
RecoJets/JetAlgorithms
RecoLocalMuon/DTSegment
RecoParticleFlow/PFClusterProducer
RecoParticleFlow/PFProducer
RecoTauTag/RecoTau
RecoTracker/TkDetLayers
SimCalorimetry/HcalTrigPrimAlgos
SimG4CMS/ShowerLibraryProducer
SimG4Core/Geometry
SimTracker/SiStripDigitizer
SimTracker/TrackAssociation
TopQuarkAnalysis/TopKinFitter
Validation/EcalDigis

@andrius-k, @lveldere, @sbein, @schneiml, @ianna, @kpedro88, @rekovic, @fioriNTU, @tlampen, @pohsun, @santocch, @perrotta, @civanch, @makortel, @cmsbuild, @smuzaffar, @Dr15Jones, @cvuosalo, @ssekmen, @mdhildreth, @jfernan2, @tocheng, @slava77, @ggovi, @benkrikler, @kmaeshima, @christopheralanwest, @alja can you please review it and eventually sign? Thanks.
@echabert, @rappoccio, @gouskos, @yslai, @felicepantaleo, @jainshilpi, @schoef, @robervalwalsh, @emilbols, @missirol, @argiro, @Martin-Grunewald, @Fedespring, @thomreis, @tlampen, @yenjie, @ahinzmann, @lgray, @abbiendi, @varuns23, @slomeo, @sobhatta, @mmarionncern, @smoortga, @alja, @makortel, @mtosi, @peruzzim, @mandrenguyen, @cericeci, @jhgoh, @dgulhan, @jdolen, @prolay, @JyothsnaKomaragiri, @ebrondol, @seemasharmafnal, @cbernet, @ferencek, @rociovilar, @pieterdavid, @forthommel, @yetkinyilmaz, @Sam-Harper, @barvic, @DryRun, @afiqaize, @GiacomoSguazzoni, @tocheng, @VinInn, @jdamgov, @bellan, @nhanvtran, @gkasieczka, @rovere, @threus, @hatakeyamak, @mschrode, @mmusich, @abdoulline, @fabiocos, @clelange, @kreczko, @HuguesBrun, @adewit, @rchatter, @trocino, @riga, @battibass, @gbenelli, @jazzitup, @calderona, @mverzett, @dkotlins, @lecriste, @kurtejung, @gpetruc, @matt-komm, @mariadalfonso, @amarini, @andrzejnovak, @folguera, @jbsauvan this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@smuzaffar smuzaffar modified the milestones: CMSSW_11_1_X, CMSSW_11_2_X May 22, 2020
@pohsun
Copy link

pohsun commented May 23, 2020

+1

@santocch
Copy link

+1

@civanch
Copy link
Contributor

civanch commented May 26, 2020

+1

@jfernan2
Copy link
Contributor

+1

Copy link
Contributor

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, this is a good exercise that clearly indicates potential for code improvements.

BTW, DQM copies lots of strings. Perhaps, using string_views would help with performance?

@@ -97,6 +97,8 @@ CTPPSFastTrackingProducer::CTPPSFastTrackingProducer(const edm::ParameterSet& iC

//Timing Detector Description
std::vector<double> vToFCellWidth;
vToFCellWidth.reserve(8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a one-line would do:

// Declaring new vector and copying 
// element of old vector 
// constructor method, Deep copy 
std::vector<double> vToFCellWidth(fToFCellWidth);

@@ -351,6 +353,8 @@ void CTPPSFastTrackingProducer::FastReco(int Direction, H_RecRPObject* station)
double pos_tof = fToFInsertion * fBeamXRMS_ToF + fToFXOffset;
int cellId = 0;
std::vector<double> vToFCellWidth;
vToFCellWidth.reserve(8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

std::vector<double> vToFCellWidth(fToFCellWidth);

@@ -222,6 +222,8 @@ void CTPPSRecHitProducer::produce(edm::Event& iEvent, const edm::EventSetup& iSe
double pos_tof = fToFInsertion * fBeamXRMS_ToF + fToFXOffset;

std::vector<double> vToFCellWidth;
vToFCellWidth.reserve(8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

std::vector<double> vToFCellWidth(fToFCellWidth);

@@ -30,6 +30,8 @@ std::vector<ME0TriggerDigi> ME0Motherboard::readoutTriggers() {
std::vector<ME0TriggerDigi> tmpV;

std::vector<ME0TriggerDigi> all_trigs = getTriggers();
tmpV.reserve(all_trigs.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

std::vector<ME0TriggerDigi> tmpV(all_trigs);

@@ -214,8 +214,12 @@ vector<int> CandOneToOneDeltaRMatcher::AlgoBruteForce(int nMin, int nMax) {
float totalDeltaR = 0;
float BestTotalDeltaR = 1000;

ca.reserve(nMax);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively:

std::vector<int> ca(nMax);
std::iota(ca.begin(), ca.end(), 0);

@@ -94,6 +94,8 @@ void HiGenCleaner<T2>::produce(edm::StreamID, edm::Event& iEvent, const edm::Eve
int jetsize = genjets->size();

vector<int> selection;
selection.reserve(jetsize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vector<int> selection(jetsize, -1);

@@ -106,6 +106,8 @@ void CalibratedPhotonProducerT<T>::fillDescriptions(edm::ConfigurationDescriptio
desc.add<bool>("produceCalibratedObjs", true);
desc.add<bool>("semiDeterministic", true);
std::vector<std::string> valMapsProduced;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string_view?

@@ -116,6 +116,8 @@ void CalibratedElectronProducerT<T>::fillDescriptions(edm::ConfigurationDescript
desc.add<bool>("produceCalibratedObjs", true);
desc.add<bool>("semiDeterministic", true);
std::vector<std::string> valMapsProduced;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string_view?

@@ -320,6 +320,8 @@ void HybridClusterAlgo::mainSearch(const EcalRecHitCollection* hits, const CaloS

std::vector<int> OwnerShip;
std::vector<double> LumpEnergy;
OwnerShip.reserve(int(dominoEnergy.size()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::vector<int> OwnerShip(int(dominoEnergy.size()), -1);

@@ -279,6 +279,8 @@ void DTLinearFit::fit4Var(const vector<float>& xfit,
int nppar3 = 0;
int nppar4 = 0;

sigy.reserve(nptfit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vector<float> sigy(nptfit, sigma);

@slava77
Copy link
Contributor

slava77 commented Jun 10, 2020

+1
sort of

(also to take this off our review list)

@mrodozov
Copy link
Contributor

  • note
    fix new line issue

@mrodozov
Copy link
Contributor

mrodozov commented Jul 1, 2020

take it in fix the issue later

@fwyzard fwyzard closed this Jul 14, 2020
@fwyzard fwyzard deleted the clang-tidy-performance-inefficient-vector-operation branch July 14, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet