Migrate legacy modules in GenFilters to global

[wddgit]

PR description:

Migrate 4 legacy modules in GeneratorInterface/GenFilters
to be global filters to improve multi threaded performance.

BCToEFilter
FourLepFilter
LHEGenericFilter
LHEVpTFilter

Includes some cleanup. (This is part 1, more of these PRs are coming
and they will repeat the patterns in this one. If there is something you
do not like, please let me know.)

PR validation:

Relies on existing tests. Output should not change, except that some
output to log files containing a count of events and events passed
is deleted.

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28946/13758

• This PR adds an extra 20KB to repository

[cmsbuild]

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

GeneratorInterface/GenFilters

@SiewYan, @efeyazgan, @mkirsano, @cmsbuild, @agrohsje, @alberto-sanchez, @qliphy can you please review it and eventually sign? Thanks.
@alberto-sanchez, @agrohsje, @mkirsano this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

[wddgit]

please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4635/console Started: 2020/02/13 07:53

[cmsbuild]

+1
Tested at: 1e7e15f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7a605a/4635/summary.html
CMSSW: CMSSW_11_1_X_2020-02-12-2300
SCRAM_ARCH: slc7_amd64_gcc820

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7a605a/4635/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: 2694005
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2693658
• DQMHistoTests: Total skipped: 346
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
• Checked 147 log files, 16 edm output root files, 34 DQM output files

[cmsbuild]

Pull request #28946 was updated. @SiewYan, @efeyazgan, @mkirsano, @cmsbuild, @agrohsje, @alberto-sanchez, @qliphy can you please check and sign again.

[wddgit]

please test

rebased plus updated to be more careful about std::abs usage. I'm not sure that it was broken either by this PR or before, but make sure the needed headers are there and it's always looking in std:: for the overloads with no dependence on includes from includes.

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4799/console Started: 2020/02/20 18:57

[cmsbuild]

+1
Tested at: f694dae
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7a605a/4799/summary.html
CMSSW: CMSSW_11_1_X_2020-02-19-2300
SCRAM_ARCH: slc7_amd64_gcc820

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7a605a/4799/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: 2694086
• DQMHistoTests: Total failures: 2
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2693765
• DQMHistoTests: Total skipped: 319
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
• Checked 147 log files, 16 edm output root files, 34 DQM output files

[silviodonato]

@alberto-sanchez @agrohsje @efeyazgan @mkirsano @qliphy @SiewYan
do you have any comments?

[agrohsje]

Looks good to me. Just one question. The old code kept track of the efficiency. Not sure this is used by someone, but I couldn't find the same info in the updated code. Is this still available?

[wddgit]

The Framework provides something like this for free. Here is an example. Run

FWCore/​Framework/​test/​testBitsCount_cfg.py

Here are selected lines from the output log file:

...
TrigReport ---------- Module Summary ------------
TrigReport    Visited   Executed     Passed     Failed      Error Name
...
TrigReport         99         99          3         96          0 f1
TrigReport         99         99          1         98          0 f2
TrigReport         99         99          8         91          0 f3
TrigReport         99         99         30         69          0 f4
TrigReport         99         99         70         29          0 f5
TrigReport         99         99         12         87          0 f6
...

where f1, f2, f3, f4, f5, and f6 are the module labels of some
filters in the test. If you look in the configuration, this is the
part that turns it on.

process.options = cms.untracked.PSet(
    wantSummary = cms.untracked.bool(True)
)

The only thing that is missing is the efficiency itself which
can easily be calculated from the passed and executed columns.
The only other weaknesses of this compared to what was removed
from the modules is that this enables a significant amount of printout
in the log file and you have find your module label, plus you have
to remember to enable it in the configuration.

The downside of the counters that were in the modules is
that they are not const. In a global module, simple "int" counters
would not work. One could make them std::atomic's
and make that work, but that seems like a lot of overhead
for something the Framework provides for free. You are adding
some performance degradation when you add an atomic because
it has to be synchronized as the job runs. Also it clutters the code.
If you really think these are important, I will put them back as atomics
(assuming no one else objects). But I think the better solution is
just to remove them.

I'll also note some modules in GenFilters have these counters and some
don't and the printout is inconsistent, so it is not implemented in
a uniform way in the package...

[agrohsje]

Thanks @wddgit .
+1

[wddgit]

@agrohsje Are you the generators official approver? For some reason, it says it still needs approval? Maybe the +1 needs to be on the first line? Thanks.

[qliphy]

+1

[cmsbuild]

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

[silviodonato]

+1