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.

Sign up for GitHub

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

Jump to bottom

Traj float #5431

Merged
merged 12 commits into from Sep 22, 2014
Merged

Traj float #5431

merged 12 commits into from Sep 22, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Sep 19, 2014

This PR contains two components:
1)
change double to float in LocalTrajectoryParameters.
for TTBAR 40PU25ns I observe a reduction of ~100MB of RSS for a very-small regression and no changes in tracking performances
No clue why was not done before (most probably less relevant at low pileup)

  1. add "pt" to PTrajectoryStateOnDet
    the original idea was to be able to access the "pt" of a seed w/o converting to a TSOS
    it turns out that sorting seed in pt does not improve anything.
    I prefer to keep it around for a couple of pre-releases, eventually remove (and clean code) if not useful in any way

@cmsbuild cmsbuild added this to the Next CMSSW_7_3_X milestone Sep 19, 2014
@cmsbuild
Copy link
Contributor

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

Traj float

It involves the following packages:

DataFormats/TrajectoryState
FastSimulation/Muons
FastSimulation/Tracking
RecoLocalTracker/SubCollectionProducers
RecoTracker/CkfPattern
TrackingTools/TrajectoryState

@civanch, @nclopezo, @lveldere, @mdhildreth, @cmsbuild, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
@ghellwig, @abbiendi, @battibass, @threus, @forthommel, @yduhm, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @nickmccoll, @bellan, @jlagram, @gpetruc, @cerati, @trocino, @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.

@slava77
Copy link
Contributor

slava77 commented Sep 19, 2014

Hi Vincenzo,

Is this reduction of 100MB expected from the first principles,
as in we do have ~ 25M instances of doubles in the
LocalTrajectoryParameters kind?

On 9/19/14, 4:14 AM, Vincenzo Innocente wrote:

This PR contains two components:
1)
change double to float in LocalTrajectoryParameters.
for TTBAR 40PU25ns I observe a reduction of ~100MB of RSS for a
very-small regression and no changes in tracking performances
No clue why was not done before (most probably less relevant at low pileup)

  1. add "pt" to PTrajectoryStateOnDet
    the original idea was to be able to access the "pt" of a seed w/o
    converting to a TSOS
    it turns out that sorting seed in pt does not improve anything.
    I prefer to keep it around for a couple of pre-releases, eventually
    remove (and clean code) if not useful in any way
    You can merge this Pull Request by running

git pull https://github.com/VinInn/cmssw TrajFloat

Or view, comment on, or merge it at:

#5431

    Commit Summary

—
Reply to this email directly or view it on GitHub
#5431.

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

@VinInn
Copy link
Contributor Author

VinInn commented Sep 19, 2014

Indeed, I hope we do not have 5M LocalTrajectoryParameters objects floating around (we can count them...)
On the other hand each seed has one of those and we can have easily up to 100K seeds.
there is a factor 50 missing. can be investigated more.
btw it was running with 8 threads

@VinInn
Copy link
Contributor Author

VinInn commented Sep 19, 2014

I counted them
LocalTrajectoryParameters 0 1153579
so running with 8 threads we have a maximum of 1.15M LocalTrajectoryParameters objects alive at the same time!
the fact that the total number is "0" at the end supports that I did not make major mistakes
(the first round was negative, forgot the copy constructor...)

btw today with CMSSW_7_3_X_2014-09-18-0200
the max RSS running only tracker is ~1.7GB
it was ~1.55GB one week ago.
same machine, same config... (and quite consistent among various runs)

@cmsbuild
Copy link
Contributor

+1
Tested at: 35ef3af
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-5431/23/summary.html

@lveldere
Copy link
Contributor

+1

@slava77
Copy link
Contributor

slava77 commented Sep 21, 2014

testing

slava77
slava77 reviewed Sep 21, 2014
View reviewed changes
RecoTracker/CkfPattern/src/CkfTrackCandidateMakerBase.cc
// to be moved inside a par section
vector<Trajectory> theTmpTrajectories;

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

better get rid of it now, unless the point is to keep this as a somewhat obvious thing that doesn't work for future generations.

@slava77
Copy link
Contributor

slava77 commented Sep 22, 2014

+1

for #5431 35ef3af
tested in CMSSW_7_3_X_2014-09-19-1400 (test area sign432)

Overall, no significant adverse changes in physics outputs, as expected from using floats instead of doubles. More numerically complicated outputs are affected slightly more: general tracks are almost unchanged, gsf/conversions are affected more; and even a bit more variation in the b-tag variables for single-particle guns (most num unstable parts).

Based on MemoryCheck report in ttbar with run-1 PU running only RAW2DIGI,L1Reco,RECO,EI steps for 200 events: there doesn't seem to be much change in the peak memory usage

  • on 100th event: VSIZE goes down by 2MB, while RSS remains the same
  • on the last event: VSIZE goes down by ~16MB (1895.57), RSS goes up by 16MB (1527.05 0)
    By construction, there is less memory allocated, but I guess the way how pages show up and how pre-allocation happens makes the differences smaller.

There seem to be some reduction in CPU, but this may be from some DB latencies

        generalTracks    25.6961 ms/ev -> 22.3555 ms/ev
        allConversions   61.852 ms/ev -> 56.3334 ms/ev

The above (wall-clock times) are in agreement with perf decreases with igprof.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

davidlange6 added a commit that referenced this pull request Sep 22, 2014
@davidlange6
Merge pull request #5431 from VinInn/TrajFloat
83d592b 
Traj float
@davidlange6 davidlange6 merged commit 83d592b into cms-sw:CMSSW_7_3_X Sep 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
comparison-available fastsim-approved fully-signed reconstruction-approved tests-approved
Projects
None yet
Milestone
Next CMSSW_7_3_X
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@VinInn @cmsbuild @slava77 @lveldere @davidlange6