Fix ordering bug in reworked PFBlockAlgo

[lgray]

This ordering bug was causing crashes in relvals for 810pre4.

I have checked in a few cases that the exceptions are no longer thrown.
Notably, in these cases more blocks were being created than with the old algorithm.
With the bugfix provided here, the old behavior is restored.

[lgray]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12906/console

[cmsbuild]

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_8_1_X.

It involves the following packages:

RecoParticleFlow/PFProducer

@cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@mmarionncern, @rafaellopesdesa, @bachtis, @cbernet this is something you requested to watch as well.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

[cmsbuild]

+1
Tested at: de8fa67
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14441/12906/summary.html

[cmsbuild]

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

[slava77]

+1

for #14441 de8fa67

• code changes are in line with the description and preceding email exchange
• jenkins tests pass and comparisons with baseline show numerous small differences starting from pfBlock and then products downstream
• local tests with a bit more statistics show similar differences to those observed in jenkins comparisons
• crash reported in the relval workflows https://hypernews.cern.ch/HyperNews/CMS/get/relval/4897.html
  is gone
• this is probably not the end of the follow-ups to changes in pfAlgo made in pre4 : there is still a valgrind error of uninitialized condition in PFBlockAlgo::findBlocks()

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

[lgray]

Hi Slava, can you point me to the information from valgrind? Thanks.

[slava77]

I was running 1001.0 in CMSSW_8_1_X_2016-05-02-2300 (I suppose, it would be the same in pre4)

  <kind>UninitCondition</kind>
      PFBlockAlgo::findBlocks()
      <dir>RecoParticleFlow/PFProducer/src</dir>
      <file>PFBlockAlgo.cc</file>
      <line>192</line>

it's

 if( *pos != key || 0 == keys.size() ) {

I don't see how keys.size can be uninitialized; so, it may be pos or key; but then I wasn't able to make sure of it with -O0 (valgrind crashes on code recompiled with -O0). So, given that it's after optimization, the line number may be misleading.

It's unclear to me where it's coming upstream
I tried to put random numbers in malloc and asserts in obvious places (run cmsRunGlibC to use malloc), but that was probably not good enough.

[lgray]

Ah, *pos may dereference keys.end(). This is fixed merely by changing the order of the clauses in the if statement. I can patch it up quickly right now if you're OK to resign?

[slava77]

ok, please

[lgray]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/12927/console

[cmsbuild]

Pull request #14441 was updated. @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please check and sign again.

[cmsbuild]

+1
Tested at: 6bb7d52
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-14441/12927/summary.html

[cmsbuild]

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

[slava77]

+1

for #14441 6bb7d52

• changes are as desired
• jenkins tests pass and comparisons show small differences starting from PFBlock
• local tests comparing 6bb7d52 vs de8fa67 show that the valgrind error of UninitCondition in PFBlockAlgo is gone; there are also single-bin differences in PF starting from PFBlock from this last update

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar

[davidlange6]

+1