Navigation #3547
Navigation #3547
Conversation
|
It breaks BeamHalo (wmf 8.0) |
|
A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_1_X. Navigation It involves the following packages: RecoEgamma/EgammaPhotonProducers @nclopezo, @cmsbuild, @anton-a, @thspeer, @slava77, @Degano can you please review it and eventually sign? Thanks. |
| #include "FWCore/MessageLogger/interface/MessageLogger.h" | ||
|
|
||
| #include <iostream> | ||
| #include <iomanip> | ||
| using namespace std; | ||
|
|
||
| MuonNavigationPrinter::MuonNavigationPrinter(const MuonDetLayerGeometry * muonLayout, bool enableRPC) { | ||
| #define VI_DEBUG |
|
-1 please remove couts |
|
cout went out and in again due to a bad sync on my area sorry... |
|
@davidlange6 @Dr15Jones |
|
oops, needs a rebase... |
|
obsoleted by #3590 |
On Friday Chris reported that
Trackings use of DetLayer is inherently thread unsafe. The NavigationSchool is in the EventSetup but it gives non-const access to its internal DetLayers:
https://cmssdt.cern.ch/SDT/lxr/source/TrackingTools/DetLayers/interface/NavigationSchool.h?v=CMSSW_7_1_X_2014-04-25-0200#028
and those DetLayers are constantly being modified by other code:
https://cmssdt.cern.ch/SDT/lxr/source/TrackingTools/DetLayers/src/NavigationSetter.cc?v=CMSSW_7_1_X_2014-04-25-0200#059
In addition, the DetLayers have internally a NavigableLayer* which they give non-const access to and call non-const methods on in its own const functions:
https://cmssdt.cern.ch/SDT/lxr/source/TrackingTools/DetLayers/interface/DetLayer.h?v=CMSSW_7_1_X_2014-04-25-0200#050
and to top it off, some types inheriting from NavigableLayer use mutables
https://cmssdt.cern.ch/SDT/lxr/source/RecoTracker/TkNavigation/interface/SimpleNavigableLayer.h?v=CMSSW_7_1_X_2014-04-25-0200#042
The only way to deal with this now is to mark all modules which interact with DetLayers as thread-unsafe.
After a week-end long of code analysis I come to the conclusion that the best solution will be:
to create a vector of NavigableLayer const * in the NavigationSchool (or equivalent)
using DetLayer::seqNum as index.
One should them to manage to get the NavigationSchool everywhere
nextLayers and compatibleLayers are invoked a trivial indirection will do the trick…
This is
a) easy to find all calls to nextLayers and compatibleLayers.
Labour intensive to percolate the navigation school. NOT error-prone
b) quite logical, it will reassign the responsibility of navigation to the Navigation!
c) No changes to configation (unless percolation fails)
Plan there're is
The infrastructure for both tracker and muon is trivial to build.
I wil do it togehter with a couple of use case.
at that point the Labour intensive part can be split in many pieces: it it just to find the way to percolate navigation scool and modify the calling sequence to
nextLayers and compatibleLayers by few characters (litterally!)
remove all “mutable” including the Navigation Setter.
add const here and there (maybe a couple of cost_cast in the NavigationSchool itself)
DONE…
status : this PR
done for Base class, Simple, Cosmic and Muon
all three tested with properly modified already existing unit tests
done in CkfTrajectoryBuilder and Maker (+ partial in Conversion f/w/c TrBuilder)
one NavigationSetter gone!
at this point we ca go in Parallel.
This PR is therefore requested to be integrated to allow other developers to modify various bits of code
that require NavigationSchool to be percolated
No regression expected
No regression observed