Pr80x l1t memory fixes #16085

rekovic · 2016-10-04T11:58:36Z

This PR contains 3 types of fixes concerning memory management by L1Trigger:

fix for memory leak in Muon Barell Extrapolater which was introduced with merging of O2O
(free allocated memory before every return). This is already provided in 81x Fix memory leak in L1Trigger/L1TMuonBarrel/src/L1MuBMEUX #16061 by @davidlange6
fix for repetitive memory allocation in Muon Overlap XML Parser to reset DOM vector pool and release all the associated memory back to the system before re-using read methods.
fix for Invalid writes in Muon Barell ParamsHelper class by reserving the correct vector size.

…merge.

…r, (2) release memory from parser back to system at the end of 'read' methods.

… in L1TMuonBarrelParamsHelper.

cmsbuild · 2016-10-04T11:58:51Z

A new Pull Request was created by @rekovic for CMSSW_8_0_X.

It involves the following packages:

L1Trigger/L1TMuonBarrel
L1Trigger/L1TMuonOverlap

@cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

rekovic · 2016-10-04T12:01:08Z

please test

cmsbuild · 2016-10-04T12:01:19Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15513/console

rekovic · 2016-10-04T13:50:26Z

Some info on the change of memory usage by OMTF XMLConfigReader:

Before fix: OMTF XMLConfigReader has no destructor (i.e. no deleting of parser), but delete DOM documents at the end of read methods (multiple calls):
```
PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
7342 rekovic   20   0 2326m 1.4g 121m R 100.1  6.0   2:23.00 cmsRun  
```

After adding destructor and reseting parser's document pool in read methods:

PID USER      PR  NI  VIRT  RES  SHR S %CPU %MEM    TIME+  COMMAND
12196 rekovic   20   0 1922m 922m 121m R 97.5  3.9   2:12.69 cmsRun

Reduction in actual physical memory consumption (in RES) is about 500m.

cmsbuild · 2016-10-04T15:46:38Z

+1
Tested at: 49bd6ad
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15513/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
1c1b498
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15513/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15513/git-merge-result

cmsbuild · 2016-10-04T16:30:45Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15513/summary.html

rekovic · 2016-10-04T19:38:04Z

+1

cmsbuild · 2016-10-04T19:38:29Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

cmsbuild · 2016-10-06T08:35:26Z

Pull request #16085 was updated. @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please check and sign again.

davidlange6 · 2016-10-06T08:46:04Z

L1Trigger/L1TMuonBarrel/src/L1TMuonBarrelParamsHelper.cc

@@ -35,8 +35,8 @@ void L1TMuonBarrelParamsHelper::configFromPy(std::map<std::string, int>& allInts

 	setAssLUTPath(AssLUTpath);
 	///Read Pt assignment Luts
-	std::vector<LUT> pta_lut(0); pta_lut.reserve(19);
-	std::vector<int> pta_threshold(6); pta_threshold.reserve(10);
+	std::vector<LUT> pta_lut(19);


@rekovic - careful - I believe that pta_lut should be reserve not resize - see that there is a push_back (but only 1 push_back.. ) on line 379 (this code is extremely confusing)

please confirm...

@davidlange6 Yes, you are right. Pushing back on pta_lut vector will place beyond index 18,
which is not good. Changing back to reserve for pta_lut.

…k().

cmsbuild · 2016-10-06T09:25:28Z

Pull request #16085 was updated. @cmsbuild, @rekovic, @mulhearn, @davidlange6 can you please check and sign again.

rekovic · 2016-10-06T09:30:07Z

please test

rekovic · 2016-10-06T09:30:12Z

+1

cmsbuild · 2016-10-06T09:30:19Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/15560/console

cmsbuild · 2016-10-06T09:30:19Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

cmsbuild · 2016-10-06T11:06:02Z

+1
Tested at: 28145c7
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15560/summary.html

cmsbuild · 2016-10-06T12:36:25Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16085/15560/summary.html

davidlange6 · 2016-10-07T14:15:23Z

+1

rekovic added 3 commits October 4, 2016 13:15

Delete what you create before every return - fix needed after O2O PR …

b7ef33e

…merge.

Reduce memory usage by OMTF XMLConfigReader: (1) introduce desctructo…

d67cf86

…r, (2) release memory from parser back to system at the end of 'read' methods.

Fix valgrind errors of Invalid writes: reserve correct size of vector…

49bd6ad

… in L1TMuonBarrelParamsHelper.

cmsbuild added this to the Next CMSSW_8_0_X milestone Oct 4, 2016

cmsbuild added comparison-pending l1-pending orp-pending pending-signatures tests-pending labels Oct 4, 2016

cmsbuild added tests-started and removed tests-pending labels Oct 4, 2016

rekovic mentioned this pull request Oct 4, 2016

Pr81x l1t memory fixes #16088

Merged

cmsbuild added tests-approved and removed tests-started labels Oct 4, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 4, 2016

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Oct 4, 2016

Declare vector with correct size, and remove reserve().

e17fb33

cmsbuild removed the comparison-available label Oct 6, 2016

cmsbuild added comparison-pending l1-pending pending-signatures tests-pending and removed tests-approved labels Oct 6, 2016

davidlange6 reviewed Oct 6, 2016

View reviewed changes

Revert to reserve() for vector which is filled with calls to push_bac…

28145c7

…k().

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Oct 6, 2016

cmsbuild added tests-started and removed tests-pending labels Oct 6, 2016

cmsbuild added tests-approved and removed tests-started labels Oct 6, 2016

cmsbuild added comparison-available and removed comparison-pending labels Oct 6, 2016

cmsbuild added orp-approved and removed orp-pending labels Oct 7, 2016

cmsbuild merged commit 3cc97a6 into cms-sw:CMSSW_8_0_X Oct 7, 2016

rekovic mentioned this pull request Oct 11, 2016

Memory leaks - L1T code in CMSSW for Sept 2016 MC Production cms-l1t-offline/cmssw#423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr80x l1t memory fixes #16085

Pr80x l1t memory fixes #16085

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016 •

edited

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 6, 2016

davidlange6 Oct 6, 2016

rekovic Oct 6, 2016

cmsbuild commented Oct 6, 2016

rekovic commented Oct 6, 2016

rekovic commented Oct 6, 2016

cmsbuild commented Oct 6, 2016 •

edited

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

davidlange6 commented Oct 7, 2016

Pr80x l1t memory fixes #16085

Pr80x l1t memory fixes #16085

Conversation

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016 • edited

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 6, 2016

davidlange6 Oct 6, 2016

Choose a reason for hiding this comment

rekovic Oct 6, 2016

Choose a reason for hiding this comment

cmsbuild commented Oct 6, 2016

rekovic commented Oct 6, 2016

rekovic commented Oct 6, 2016

cmsbuild commented Oct 6, 2016 • edited

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

davidlange6 commented Oct 7, 2016

cmsbuild commented Oct 4, 2016 •

edited

cmsbuild commented Oct 6, 2016 •

edited