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

Use shrink_to_fit on the PFRecHit collections #9106

Merged
merged 1 commit into from May 20, 2015

Conversation

mark-grimes
Copy link

Uses shrink_to_fit in PFRecHitProducer as suggested by @lgray.
Memory size of the produced collections in SLHC25_patch6 (top 25 only):
before
Same again after applying this PR and #9096:
allmemimprovements

Looking at the VmSize there appears to be an improvement. This plot is hard to read because the x-axis is approximately time, and they didn't run at the same speed. Each undulating peak is an event, it rises as control runs through the modules and then drops at the start of the next event. The red (this PR, #9096 and #9084) is a little faster (wouldn't read anything into that) so the peaks are a little ahead of the blue (SLHC25_patch6). The important thing is that if you match the corresponding peaks the red is ~100Mb lower. The vast majority of that is this pull request rather than #9096 or #9084.
vmsize_allimprovements

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mark-grimes (Mark Grimes) for CMSSW_6_2_X_SLHC.

Use shrink_to_fit on the PFRecHit collections

It involves the following packages:

RecoParticleFlow/PFClusterProducer

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @bachtis, @lgray 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@fratnikov, @mark-grimes you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@lgray
Copy link
Contributor

lgray commented May 15, 2015

@mark-grimes Can you also post a plot of RSS usage as you have for VMsize?

@mark-grimes
Copy link
Author

Here are the plots but I'm not finding RSS a good metric, it's too dependent on whatever else is happening on the node. You can see when the system load spikes for "Before" at ~6000 a big chunk of memory is paged out to disk and the RSS drops, although clearly it's not using less memory. It's only really useful if I have complete control over what's running on the node, which I don't.
rss
systemload

@lgray
Copy link
Contributor

lgray commented May 15, 2015

Fair point. But RSS is also the thing that jobs get killed for, so it's useful to keep track of it.

@mark-grimes
Copy link
Author

@fratnikov, don't merge this one yet. I'm testing using edm::RunningAverage backported from 75X to reserve the collection size. I will probably add some more commits to this PR.

@mark-grimes
Copy link
Author

I was a bit concerned about this because the peak memory use appeared to go up with the shrink_to_fit. I double checked everything and this is indeed the case, I guess the shrink_to_fit performs a copy rather than a resize, looking at the docs for realloc this is inline with the spec. Note for the memory tracking I use I need to use cmsRunGlibC, perhaps the jemalloc in cmsRun is different. I also had a go backporting edm::RunningAverage from 75X to guess a value for reserve. Results below (all 200 pileup, exact same events for each).

Memory size of the event products (both shrink_to_fit versions overlap, so the green is obscured by the purple):
pfrechithgcee_productsize

Peak memory size:
pfrechithgcee_peakmem

Whatever happens, guessing the reserve size with edm::RunningAverage is an improvement so I'll push those changes. Using shrink_to_fit is a trade between peak memory and retained memory. I'm inclined to keep the shrink_to_fit because at the moment Pandora has a higher peak memory use - so if there's enough memory to run Pandora there's enough memory to use the shrink_to_fit, and it results in more free memory to run Pandora. I'll bring this up in a simulation meeting though and ask for opinions.

@mark-grimes
Copy link
Author

#9135 backports RunningAverage and applies the reserve guess. Since there could be long discussion of the peak/retained trade off of merge_to_fit I thought it was better to have them in separately.

@cmsbuild
Copy link
Contributor

@mark-grimes
Copy link
Author

Rebased because of conflicts introduced by #9135.

@cmsbuild
Copy link
Contributor

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

@cmsbuild
Copy link
Contributor

@mark-grimes
Copy link
Author

merge

cmsbuild added a commit that referenced this pull request May 20, 2015
Use shrink_to_fit on the PFRecHit collections
@cmsbuild cmsbuild merged commit fac67e7 into cms-sw:CMSSW_6_2_X_SLHC May 20, 2015
@mark-grimes mark-grimes deleted the shrinkPFRecHit branch May 21, 2015 01:38
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

3 participants