Reduce boost regex dependencies #30333

camolezi · 2020-06-22T19:22:26Z

PR description:

Replaced boost regex for c++11 stl regex, they should have similar behavior.

PR validation:

Passed on basic runTheMatrix test.

if this PR is a backport please specify the original PR and why you need to backport that PR:

@vgvassilev @davidlange6

cmsbuild · 2020-06-22T19:22:52Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-06-22T19:31:32Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30333/16335

This PR adds an extra 104KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DQMOffline/Trigger/plugins/HLTMuonRefMethod.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935
- File DQMServices/StreamerIO/plugins/DQMFileIterator.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933
- File DQMServices/StreamerIO/plugins/RamdiskMonitor.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-for-range-copy #29927, [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933, DQM: Allow end lumi work in DQMOneEDAnalyzer #30326
- File EventFilter/L1TRawToDigi/src/MP7FileReader.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936
- File EventFilter/L1TRawToDigi/src/MP7PacketReader.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWFileEntry.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWHLTTriggerTableView.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWHLTValidator.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWL1TriggerTableView.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File HLTrigger/Timer/plugins/FastTimerServiceClient.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933
- File PhysicsTools/FWLite/src/TH1Store.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936
- File RecoLocalTracker/SiPixelRecHits/src/PixelCPEClusterRepair.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File RecoTracker/FinalTrackSelectors/plugins/TrackerTrackHitFilter.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-emplace #29937

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30333/16335/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30333/16335/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2020-06-22T19:44:26Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-06-22T19:51:19Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30333/16336

This PR adds an extra 104KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DQMOffline/Trigger/plugins/HLTMuonRefMethod.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935
- File DQMServices/StreamerIO/plugins/DQMFileIterator.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933
- File DQMServices/StreamerIO/plugins/RamdiskMonitor.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-for-range-copy #29927, [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933, DQM: Allow end lumi work in DQMOneEDAnalyzer #30326
- File EventFilter/L1TRawToDigi/src/MP7FileReader.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936
- File EventFilter/L1TRawToDigi/src/MP7PacketReader.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWFileEntry.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWHLTTriggerTableView.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWHLTValidator.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File Fireworks/Core/src/FWL1TriggerTableView.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-auto #29936, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File HLTrigger/Timer/plugins/FastTimerServiceClient.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-unnecessary-value-param #29933
- File PhysicsTools/FWLite/src/TH1Store.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936
- File RecoLocalTracker/SiPixelRecHits/src/PixelCPEClusterRepair.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File RecoTracker/FinalTrackSelectors/plugins/TrackerTrackHitFilter.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-emplace #29937

cmsbuild · 2020-06-22T19:51:43Z

A new Pull Request was created by @camolezi (Lucas Camolezi) for master.

It involves the following packages:

CommonTools/Utils
CondCore/CondDB
DQMOffline/Trigger
DQMServices/StreamerIO
DataFormats/PatCandidates
EventFilter/L1TRawToDigi
FWCore/Framework
Fireworks/Core
HLTrigger/Timer
PhysicsTools/FWLite
RecoLocalTracker/SiPixelRecHits
RecoTracker/FinalTrackSelectors

@perrotta, @smuzaffar, @benkrikler, @Dr15Jones, @alja, @makortel, @fwyzard, @schneiml, @andrius-k, @cmsbuild, @rekovic, @jfernan2, @kmaeshima, @fioriNTU, @Martin-Grunewald, @slava77, @ggovi, @santocch can you please review it and eventually sign? Thanks.
@gouskos, @felicepantaleo, @Martin-Grunewald, @wddgit, @thomreis, @threus, @mmusich, @cericeci, @calderona, @makortel, @JanFSchulte, @jhgoh, @peruzzim, @missirol, @HuguesBrun, @trocino, @rociovilar, @barvic, @GiacomoSguazzoni, @rovere, @VinInn, @hatakeyamak, @mschrode, @ebrondol, @mtosi, @dgulhan, @Fedespring, @alja, @dkotlins, @cbernet, @gpetruc, @folguera this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

alja · 2020-06-22T20:23:06Z

+1

cmsbuild · 2020-06-22T20:23:31Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-06-22-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7259/console Started: 2020/06/22 22:24

makortel · 2020-06-22T20:44:01Z

FWCore/Framework/doc/EventSelector.cc-with-cerr-traces

@@ -13,7 +13,7 @@
 #include "FWCore/Utilities/interface/Algorithms.h"

 #include "boost/algorithm/string.hpp"
-#include "boost/regex.hpp"
+#include <regex>


This file is obsolete. I suggest we let this change go in and then I remove the file (among others). Unless this PR needs to be modified for other reasons, in which case I'd ask to leave this file untouched (but only in that case).

makortel · 2020-06-22T20:44:22Z

+core

cmsbuild · 2020-06-22T21:15:02Z

-1

Tested at: c9f59b4

CMSSW: CMSSW_11_2_X_2020-06-22-1100
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-53c91e/7259/summary.html

I found follow errors while testing this PR

Failed tests: Build HeaderConsistency

Build:

I found compilation error when building:

>> Package CondTools/SiPixel built
>> Entering Package CondCore/AlignmentPlugins
Entering library rule at src/CondCore/AlignmentPlugins/plugins
>> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/AlignmentPlugins/plugins/TrackerSurfaceDeformations_PayloadInspector.cc
In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/AlignmentPlugins/plugins/TrackerSurfaceDeformations_PayloadInspector.cc:12:
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:253:12: error: 'set' in namespace 'std' does not name a template type
       std::set m_inputParams;
            ^~~
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:253:7: note: 'std::set' is defined in header ''; did you forget to '#include '?
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:14:1:
+#include

cmsbuild · 2020-06-22T21:15:04Z

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

fwyzard · 2020-06-23T08:02:21Z

HLTrigger/Timer/plugins/FastTimerServiceClient.cc

@@ -3,7 +3,7 @@
 #include <cstring>

 // boost headers
-#include <boost/regex.hpp>
+#include <regex>


Could you move this header to the "C++ headers" section, and remove the now empty "boost headers" section ?

fwyzard · 2020-06-23T08:02:33Z

HLTrigger/Timer/plugins/ThroughputServiceClient.cc

@@ -3,7 +3,7 @@
 #include <cstring>

 // boost headers
-#include <boost/regex.hpp>
+#include <regex>


Could you move this header to the "C++ headers" section, and remove the now empty "boost headers" section ?

fwyzard · 2020-06-23T08:03:11Z

PhysicsTools/FWLite/src/TH1Store.cc

@@ -4,7 +4,7 @@
 #include <iomanip>
 #include <cassert>



Suggested change

fwyzard · 2020-06-23T08:04:38Z

CommonTools/Utils/src/TFileDirectory.cc

@@ -3,7 +3,7 @@

 #include "CommonTools/Utils/interface/TFileDirectory.h"

-#include "boost/regex.hpp"
+#include <regex>


Could you move this header with the other standard headers above ?

fwyzard · 2020-06-23T08:05:00Z

CondCore/CondDB/interface/Utils.h

@@ -13,7 +13,7 @@
 #include <pwd.h>
 #include <climits>
 //


Suggested change

//

fwyzard · 2020-06-23T08:07:34Z

EventFilter/L1TRawToDigi/interface/MP7FileReader.h

@@ -16,7 +16,7 @@
 #include <vector>

 // Boost Headers
-#include <boost/regex.hpp>
+#include <regex>


Could you move this header with the other standard headers above ?

fwyzard · 2020-06-23T08:07:49Z

EventFilter/L1TRawToDigi/src/MP7FileReader.cc

@@ -4,7 +4,7 @@
 #include "EventFilter/L1TRawToDigi/interface/MP7FileReader.h"

 #include <boost/algorithm/string.hpp>
-#include <boost/regex.hpp>
+#include <regex>


Could you move this header with the other standard headers below ?

camolezi · 2020-06-25T13:40:37Z

I don't think we should go on with this replacement anymore, unless performance is not a concern.

I've made some benchmarks on std::regex vs boost::regex.
They show that std::regex is at least 10x slower than boost::regex.

https://gist.github.com/camolezi/87283aaf7cdeb584ba294588a5d77abd
https://gist.github.com/camolezi/ae83ce140a5ade6e9c4aee158f9f95e2

makortel · 2020-06-25T13:45:13Z

@camolezi Interesting (and quick internet searches support that). I'd be curious if adding -O2 would make any difference?

camolezi · 2020-06-25T14:11:33Z

@camolezi Interesting (and quick internet searches support that). I'd be curious if adding -O2 would make any difference?

wow, It made a big difference. The performance difference is much more reasonable with -O2 flag.
In the regex_match benchmark, with -O2 flag.

boost_regex: 2805ns - iterations: 250338
std_regex: 2214ns - iterations: 312415

edit: it seems that std_regex is faster with -O2 flag. I benchmarked again a couple of times and this result seems right.

camolezi · 2020-06-25T17:13:57Z

@camolezi Interesting (and quick internet searches support that). I'd be curious if adding -O2 would make any difference?

wow, It made a big difference. The performance difference is much more reasonable with -O2 flag.
In the regex_match benchmark, with -O2 flag.

boost_regex: 2805ns - iterations: 250338
std_regex: 2214ns - iterations: 312415

edit: it seems that std_regex is faster with -O2 flag. I benchmarked again a couple of times and this result seems right.

Okay, my regex_match benchmarks do show that std_regex is comparable to boost regex.
But even with optimizations, in my other benchmark (regex_seach), still suggest a huge difference in performance.

Based on this:
https://www.boost.org/doc/libs/1_73_0/libs/regex/doc/html/boost_regex/background/performance.html

It seems like in some specific cases std::regex does have a comparable performance to boost::regex. But in general, it seems like std::regex is significantly slower.

fwyzard · 2020-06-25T19:13:14Z

Interesting - thanks a lot for the investigation.

I am, of course, in favour of staying with the better-performing option :-)

replaced boost regex dependency for stl regex

6b94907

cmsbuild added this to the CMSSW_11_2_X milestone Jun 22, 2020

cmsbuild added analysis-pending code-checks-pending comparison-pending core-pending db-pending dqm-pending hlt-pending l1-pending orp-pending pending-signatures reconstruction-pending tests-pending visualization-pending labels Jun 22, 2020

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jun 22, 2020

fix code format

c9f59b4

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jun 22, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 22, 2020

cmsbuild added visualization-approved and removed visualization-pending labels Jun 22, 2020

cmsbuild added tests-started and removed tests-pending labels Jun 22, 2020

makortel reviewed Jun 22, 2020

View reviewed changes

cmsbuild added core-approved and removed core-pending labels Jun 22, 2020

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Jun 22, 2020

fwyzard suggested changes Jun 23, 2020

View reviewed changes

cmsbuild mentioned this pull request Jun 23, 2020

DQM: Allow end lumi work in DQMOneEDAnalyzer #30326

Closed

camolezi closed this Jun 25, 2020

makortel mentioned this pull request Jul 7, 2020

Remove obsolete files from FWCore/Framework/doc #30567

Merged

davidlange6 mentioned this pull request Oct 31, 2021

Replace boost/regex.hpp with standard library regex #35919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce boost regex dependencies #30333

Reduce boost regex dependencies #30333

camolezi commented Jun 22, 2020 •

edited

Loading

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

alja commented Jun 22, 2020

cmsbuild commented Jun 22, 2020 •

edited

Loading

makortel Jun 22, 2020

makortel commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

fwyzard Jun 23, 2020

camolezi commented Jun 25, 2020

makortel commented Jun 25, 2020

camolezi commented Jun 25, 2020 •

edited

Loading

camolezi commented Jun 25, 2020

fwyzard commented Jun 25, 2020

Reduce boost regex dependencies #30333

Reduce boost regex dependencies #30333

Conversation

camolezi commented Jun 22, 2020 • edited Loading

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

alja commented Jun 22, 2020

cmsbuild commented Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

makortel commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

cmsbuild commented Jun 22, 2020

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

camolezi commented Jun 25, 2020

makortel commented Jun 25, 2020

camolezi commented Jun 25, 2020 • edited Loading

camolezi commented Jun 25, 2020

fwyzard commented Jun 25, 2020

camolezi commented Jun 22, 2020 •

edited

Loading

cmsbuild commented Jun 22, 2020 •

edited

Loading

camolezi commented Jun 25, 2020 •

edited

Loading