Migrate EventFilter from boost::format to {fmt} #30855

fwyzard · 2020-07-21T17:34:33Z

PR description:

Migrate from boost::format to fmt::format or fmt::sprintf.

cmsbuild · 2020-07-21T17:34:56Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-21T17:40:26Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30855/17212

This PR adds an extra 36KB to repository

cmsbuild · 2020-07-21T17:40:51Z

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

It involves the following packages:

EventFilter/EcalRawToDigi
EventFilter/SiStripRawToDigi

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@rchatter, @makortel, @argiro, @Martin-Grunewald, @VinInn this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2020-07-21T17:42:50Z

Just a suggestion, now that {fmt} is available in CMSSW.

fwyzard · 2020-07-21T17:43:25Z

@cmsbuild, please test

cmsbuild · 2020-07-21T17:43:45Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-21-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8192/console Started: 2020/07/21 19:44

cmsbuild · 2020-07-21T18:50:07Z

+1
Tested at: 3ce563b
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-66fb5a/8192/summary.html
CMSSW: CMSSW_11_2_X_2020-07-21-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-21T18:50:09Z

Comparison job queued.

cmsbuild · 2020-07-21T20:35:47Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2525996
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2525942
DQMHistoTests: Total skipped: 47
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 144 log files, 17 edm output root files, 34 DQM output files

slava77 · 2020-07-22T14:43:47Z

@rchatter
please check or pass to the original developers where available to confirm that there are no major issues with this update

slava77 · 2020-07-23T14:08:20Z

@pieterdavid @robervalwalsh
please take a look at the SiStripRawToDigiUnpacker updates

slava77 · 2020-07-23T14:09:29Z

@pieterdavid @robervalwalsh
please take a look at the SiStripRawToDigiUnpacker updates

shouldn't you also subscribe to be notified about changes in this package?
https://github.com/cms-sw/cms-bot/blob/master/watchers.yaml

pieterdavid · 2020-07-23T14:26:56Z

@pieterdavid @robervalwalsh
please take a look at the SiStripRawToDigiUnpacker updates

shouldn't you also subscribe to be notified about changes in this package?
https://github.com/cms-sw/cms-bot/blob/master/watchers.yaml

Yes, it seems we forgot to include this package in our list - I submitted cms-sw/cms-bot#1345 to fix that. Thanks for spotting this.

I'm not very familiar with the syntax of {fmt}, but at first sight the changes look sensible.

fwyzard · 2020-07-23T14:33:57Z

{fmt} supports different syntax:

fmt::sprintf() uses the traditional printf syntax;
fmt::format() uses the C++ std::format syntax/

I migrated SiStripRawToDigiUnpacker to the latter since it was already using positional arguments and no format conversions.

cmsbuild · 2020-07-23T16:46:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-23T16:52:58Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30855/17272

This PR adds an extra 36KB to repository

cmsbuild · 2020-07-23T16:53:25Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-23-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8252/console Started: 2020/07/23 18:56

cmsbuild · 2020-07-23T16:53:26Z

Pull request #30855 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

cmsbuild · 2020-07-23T18:02:04Z

+1
Tested at: 6e42671
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-66fb5a/8252/summary.html
CMSSW: CMSSW_11_2_X_2020-07-23-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-23T18:02:07Z

Comparison job queued.

rchatter · 2020-07-23T18:44:58Z

@rchatter
please check or pass to the original developers where available to confirm that there are no major issues with this update

Hi all,

Sorry for the delay in following up. I have cross-checked with ECAL RECO experts the changes made in this file: EventFilter/EcalRawToDigi/plugins/MatacqProducer.cc .

While am not aware of this library fmt, this change seems to
transparently replace the two lines using boost formatting earlier. We don't expect any issues with this update, so from ECAL we are fine with this.

Best,
Rajdeep for the ECAL Reco team

cmsbuild · 2020-07-23T20:00:59Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2526188
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2526140
DQMHistoTests: Total skipped: 47
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 144 log files, 17 edm output root files, 34 DQM output files

jpata · 2020-07-24T08:35:21Z

I ran a quick timing test on 1302.18 and interestingly, with this PR enabled it's is very slightly slower. This is somewhat surprising, given the reportedly good performance of fmt::format. Let me know if I missed something.

$ grep "TimeReport       event loop CPU/event" */1302*/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log*
new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log:TimeReport       event loop CPU/event = 5.321165
new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log2:TimeReport       event loop CPU/event = 5.281827
new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log3:TimeReport       event loop CPU/event = 5.340586
orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log:TimeReport       event loop CPU/event = 5.256085
orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log2:TimeReport       event loop CPU/event = 5.245805
orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log3:TimeReport       event loop CPU/event = 5.275022

davidlange6 · 2020-07-24T14:52:44Z

What does something like igprof say for how much time is spent inside the library? On Jul 24, 2020, at 10:35 AM, Joosep Pata <notifications@github.com<mailto:notifications@github.com>> wrote: I ran a quick timing test on 1302.18 and interestingly, with this PR enabled it's is very slightly slower. This is somewhat surprising, given the reportedly good performance of fmt::format. Let me know if I missed something. $ grep "TimeReport event loop CPU/event" */1302*/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log* new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log:TimeReport event loop CPU/event = 5.321165 new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log2:TimeReport event loop CPU/event = 5.281827 new/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log3:TimeReport event loop CPU/event = 5.340586 orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log:TimeReport event loop CPU/event = 5.256085 orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log2:TimeReport event loop CPU/event = 5.245805 orig/1302.18_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18/step3_ProdTTbar_13UP18+ProdTTbar_13UP18+DIGIUP18PROD1+RECOPRODUP18+MINIAODMCUP18+NANOPRODUP18.log3:TimeReport event loop CPU/event = 5.275022 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#30855 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ2XOHMRNNYDHJQ5ANDR5FBV3ANCNFSM4PD2P6VQ>.

jpata · 2020-07-29T11:58:29Z

+1

tests pass
change is technical
no detectable performance difference
description is in line with the changes

It was suggested by Slava that event-level timing is not meaningful to 1%. I looked at module-level timing of the siStripDigismodule that is ran in step3, and I can't detect any significant change in performance (spread is greater than the difference). Given this bit of information, and the fact that fmt is reported to be faster than boost, I think it's fine. MatacqProducer does not seem to be used anywhere as far as I can tell.

cmsbuild · 2020-07-29T11:58:53Z

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-07-30T08:11:53Z

+1

Migrate from boost::format to {fmt}

3ce563b

cmsbuild added this to the CMSSW_11_2_X milestone Jul 21, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 21, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 21, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 21, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 21, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 21, 2020

cmsbuild added the tests-pending label Jul 23, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 23, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 23, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 23, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 23, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jul 29, 2020

cmsbuild added orp-approved and removed orp-pending labels Jul 30, 2020

cmsbuild merged commit e85a28b into cms-sw:master Jul 30, 2020

fwyzard deleted the migrate_boost_format_to_fmt_EventFilter branch August 18, 2021 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate EventFilter from boost::format to {fmt} #30855

Migrate EventFilter from boost::format to {fmt} #30855

fwyzard commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

fwyzard commented Jul 21, 2020

fwyzard commented Jul 21, 2020

cmsbuild commented Jul 21, 2020 •

edited

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

slava77 commented Jul 22, 2020

slava77 commented Jul 23, 2020

slava77 commented Jul 23, 2020

pieterdavid commented Jul 23, 2020 •

edited

fwyzard commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020 •

edited

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

rchatter commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

jpata commented Jul 24, 2020

davidlange6 commented Jul 24, 2020 via email

jpata commented Jul 29, 2020

cmsbuild commented Jul 29, 2020

silviodonato commented Jul 30, 2020

Migrate EventFilter from boost::format to {fmt} #30855

Migrate EventFilter from boost::format to {fmt} #30855

Conversation

fwyzard commented Jul 21, 2020

PR description:

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

fwyzard commented Jul 21, 2020

fwyzard commented Jul 21, 2020

cmsbuild commented Jul 21, 2020 • edited

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

cmsbuild commented Jul 21, 2020

slava77 commented Jul 22, 2020

slava77 commented Jul 23, 2020

slava77 commented Jul 23, 2020

pieterdavid commented Jul 23, 2020 • edited

fwyzard commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020 • edited

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

rchatter commented Jul 23, 2020

cmsbuild commented Jul 23, 2020

jpata commented Jul 24, 2020

davidlange6 commented Jul 24, 2020 via email

jpata commented Jul 29, 2020

cmsbuild commented Jul 29, 2020

silviodonato commented Jul 30, 2020

cmsbuild commented Jul 21, 2020 •

edited

pieterdavid commented Jul 23, 2020 •

edited

cmsbuild commented Jul 23, 2020 •

edited