Bring performance improvement in KDTreeLinkerAlgo as in #772 to RecoParticleFlow #26534
Conversation
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26534/9411
|
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages: RecoParticleFlow/PFProducer @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
I think that the common code should be moved to a common area. |
| #include "RecoParticleFlow/PFProducer/interface/KDTreeLinkerTools.h" | ||
| #include "RecoParticleFlow/PFProducer/interface/KDTreeLinkerAlgo.h" | ||
| #include "RecoPixelVertexing/PixelTriplets/plugins/KDTreeLinkerAlgo.h" | ||
| #include "RecoPixelVertexing/PixelTriplets/plugins/KDTreeLinkerTools.h" |
There was a problem hiding this comment.
cross-package includes from plugins are verboten.
I'm a bit surprised that this didn't require an update in the BuildFile.
There was a problem hiding this comment.
The KDTreeLinker* are header-only, so they don't create new link dependencies (but that doesn't make these includes any less verboten).
|
The code-checks are being triggered in jenkins. |
|
Yes you are right, I did this quickly and ran clang-format over the moved code. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26534/9412
|
|
hold I think that we should wait for #26519 to be merged |
|
Pull request has been put on hold by @slava77 |
|
unhold |
|
@guitargeek |
|
please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
@guitargeek please let us know if you plan to profile with this PR, and verify whether the improvements in performance do correspond to what you expect, or if it has to be done in our review. |
|
Hi @perrotta, sorry I was offline in the last days. I will profile this PR today myself with IgProf and 1000 events of some Run 2 TTBar workflow and report back later. |
Thank you @guitargeek |
|
Hi, the 1000 events of step 3 in workflow 11024 are done, the igprof files can be found here: https://rembserj.web.cern.ch/rembserj/igprof/26534/ Unfortunately, I was once again not able the generate the SQL files. I thought I had found an environment under which it reliably worked, but unfortunately I'm bothered again by Any idea what I could do different? Anyway, the text files are on my website and so far I can only confirm what @perrotta said before: no significant difference can be seen for the |
|
On 5/7/19 5:08 AM, Jonas Rembser wrote:
|Error: near line 63: near "�gڎ�U": syntax error|. I use the command
from the igprof website:
I have a partial solution in the reco instructions
https://twiki.cern.ch/twiki/bin/viewauth/CMS/RecoIntegration#Run_profiler_igprof
# NOTE: SOMETIMES THE ABOVE igprof-analyse --sqlite
# FAILS WITH "syntax error" due to bad characters
# additionally pipe igprof-analyse output through the following:
# sed -e 's/INSERT INTO files VALUES (\([^,]*\), \"[^$]*/INSERT INTO
files VALUES (\1, \"ABCD\");/g'
# OR dump to a txt file first and edit it manually
actually, sometimes it is enough to simply dump the result to a text
file and then to cat it to sqlite.
|
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
The particle flow code uses the k-d-tree data structure, which is also used in RecoPixelVertexing/PixelTriplets. It seems the pixel version was copy-pasted from PFProducer initially, but then significantly improved by @makortel in #772. Nice work!
I'm curious what would happen if we use this improved version in PFProducer, actually. If I understand #772 correctly, there should be no change in physics output, but 40 % faster searches. Maybe we can test it once to see if this is really the case and if yes do an igprof to see if we also benefit from the speedup? The obvious benefit would be to avoid double implementation, even if no speedup.
As the templated KDTreeLinkerAlgo and -Tools class is now used by two different reco packages, it was moved to CommonTools/RecoAlgos
Additional info: there seems to be another clone of the code in:
https://github.com/cms-sw/cmssw/blob/master/RecoLocalCalo/HGCalRecAlgos/interface/KDTreeLinkerAlgoT.h
https://github.com/cms-sw/cmssw/blob/master/RecoLocalCalo/HGCalRecAlgos/interface/KDTreeLinkerToolsT.h
That one is even more general, because it also has the dimension as a template. Maybe that development could also get incorporated in the new version in CommonTools/RecoAlgos, which is then used by PF, Pixel and HGCal. Note that there is also a more optimized version of a KDTree here, which got reimplemented from scratch:
https://github.com/cms-sw/cmssw/blob/master/CommonTools/RecoAlgos/interface/FKDTree.h
PR validation:
CMSSW compiles and matrix tests run.