Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean Propagator and TrajectoryState interface, avoid memory churn #3534

Merged
merged 18 commits into from May 3, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Apr 27, 2014

This Pull Request encompasses few related improvements

  1. Clean Propagator and TrajectoryState interface
    in particular avoids accidental conversion between "double" and "SurfaceSide" and various override issues.
  2. remove old-fashion custom intrusive-reference counting in favor of std::shared_ptr
  3. make use of std::shared_ptr ability to use custom allocator in single instances to reduce memory-churn during pattern recognition (see comments inlines)

number 3 has now a less striking effect due to the large speed-up of jemalloc itself

no regression expected
no regression observed

latest tests on top of SMatrix improvements that act also in the very same portion of code (construction of TrajectoryState in the AnalyticPropagator "called" from CkfTrajectoryBuilder)

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_1_X.

Clean Propagator and TrajectoryState interface, avoid memory churn

It involves the following packages:

Alignment/KalmanAlignmentAlgorithm
FastSimulation/Muons
RecoMuon/TrackerSeedGenerator
RecoMuon/TrackingTools
RecoPixelVertexing/PixelTrackFitting
RecoTracker/TkDetLayers
RecoVertex/GaussianSumVertexFit
RecoVertex/VertexTools
TrackPropagation/Geant4e
TrackPropagation/RungeKutta
TrackPropagation/SteppingHelixPropagator
TrackingTools/DetLayers
TrackingTools/GeomPropagators
TrackingTools/GsfTools
TrackingTools/GsfTracking
TrackingTools/KalmanUpdators
TrackingTools/MaterialEffects
TrackingTools/TrajectoryParametrization
TrackingTools/TrajectoryState

@civanch, @diguida, @lveldere, @mdhildreth, @cmsbuild, @anton-a, @thspeer, @rcastello, @giamman, @slava77, @Degano, @nclopezo can you please review it and eventually sign? Thanks.
@ghellwig, @GiacomoSguazzoni, @rovere, @gpetruc, @cerati, @bachtis, @venturia this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

destParameters.position(),
destParameters.momentum(), s);
auto const & jacobian = analyticalJacobian.jacobian();
return std::pair<TrajectoryStateOnSurface,double>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, in "standard workflows" no memory churn occur, we may try to use default allocator.

@cmsbuild
Copy link
Contributor

Pull request #3534 was updated. @civanch, @diguida, @lveldere, @mdhildreth, @cmsbuild, @anton-a, @thspeer, @rcastello, @giamman, @slava77, @Degano, @nclopezo can you please check and sign again.

1 similar comment
@cmsbuild
Copy link
Contributor

Pull request #3534 was updated. @civanch, @diguida, @lveldere, @mdhildreth, @cmsbuild, @anton-a, @thspeer, @rcastello, @giamman, @slava77, @Degano, @nclopezo can you please check and sign again.


void deallocate(pointer p, size_type n)
{
Cache & c = cache();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you guarantee that the object is deleted on the same thread as it was allocated? The framework does not give the guarantee that one event will only be processed by one thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not and does not matter.
if deleted and not found in the cache its space will not be reused.
a thread will never find an object in the cache that was NOT allocated by it.

@cmsbuild
Copy link
Contributor

-1
I found an error when building:

--- Registered EDM Plugin: TrackingToolsTrajectoryStateCapabilities
>> Leaving Package TrackingTools/TrajectoryState
>> Package TrackingTools/TrajectoryState built
@@@@ Running edmWriteConfigs for TrackingToolsGsfTrackingPlugins
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/cms/cmssw/CMSSW_7_1_X_2014-04-27-0200/lib/slc6_amd64_gcc481/libTrackingToolsTrackFitters.so: undefined reference to `TrajectoryStateOnSurface::TrajectoryStateOnSurface(LocalTrajectoryParameters const&, LocalTrajectoryError const&, Surface const&, MagneticField const_, SurfaceSideDefinition::SurfaceSide, double)'
collect2: error: ld returned 1 exit status
>> Deleted: tmp/slc6_amd64_gcc481/src/TrackingTools/GsfTracking/test/GsfMaterialEffectsUpdator_t/GsfMaterialEffectsUpdator_t
gmake: *_\* [tmp/slc6_amd64_gcc481/src/TrackingTools/GsfTracking/test/GsfMaterialEffectsUpdator_t/GsfMaterialEffectsUpdator_t] Error 1
gmake: **\* Waiting for unfinished jobs....
Leaving library rule at src/RecoPixelVertexing/PixelTrackFitting/plugins
--- Registered EDM Plugin: TrackingToolsGsfTrackingPlugins


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3534/1113/summary.html

@VinInn
Copy link
Contributor Author

VinInn commented Apr 28, 2014

sure an why the libs and not been rebuilt?
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc481/cms/cmssw/CMSSW_7_1_X_2014-04-27-0200/lib/slc6_amd64_gcc481/libTrackingToolsTrackFitters.so: undefined reference to `TrajectoryStateOnSurface::TrajectoryStateOnSurface(LocalTrajectoryParameters const&, LocalTrajectoryError const&, Surface const&, MagneticField const*, SurfaceSideDefinition::SurfaceSide, double)'

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Apr 30, 2014

looking at it ... (actually iter number 2, something went wrong with the jobs the first time, but then the tests were run combined with #3547 and all muon DQM plots were dead while having no problems with fwlite comparisons; now that #3547 is killed/superseded, will just look at this one)

@slava77
Copy link
Contributor

slava77 commented Apr 30, 2014

+1

for #3534 2ebdc26
based on a test in CMSSW_7_1_X_2014-04-30-0200 (test are sign362)
numerically unstable quantities show regressions, essentially all visible ones are in the ip2d most pronounced in "linear" events (particle guns or dijet).
I'm blaming this on the effect of O3. OK, since the effect is negligible in virtually all monitored quantities.

Here's one example (similar is in the ele gun, and pretty tiny in the dijet) for muon gun 1 TeV (wf 22.0)
all_sign362vsorig_singlemupt1000wf22p0c_recotrackiptaginfos_impactparametertaginfos__reco_obj_m_data_ip2d_value

all_sign362vsorig_singlemupt1000wf22p0c_recotrackiptaginfos_impactparametertaginfos__reco_obj_m_prob2d

@civanch
Copy link
Contributor

civanch commented May 2, 2014

+1

@VinInn
Copy link
Contributor Author

VinInn commented May 3, 2014

ping

@diguida
Copy link
Contributor

diguida commented May 3, 2014

+1
Only BuidFile.xml change (BTW, that package is to be removed...)

@ktf
Copy link
Contributor

ktf commented May 3, 2014

FastSim touched by adaptation to new API. Bypassing signature. Complain if not OK.

ktf added a commit that referenced this pull request May 3, 2014
Tracking speedup -- Clean Propagator and TrajectoryState interface, avoid memory churn
@ktf ktf merged commit 5e01867 into cms-sw:CMSSW_7_1_X May 3, 2014
@VinInn VinInn deleted the Propagate branch April 21, 2017 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants