Pr81x l1t memory fixes #16088

rekovic · 2016-10-04T13:37:35Z

This is 81x version of #16085, except for this part which is already in 81x.

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

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

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.

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

… in L1TMuonBarrelParamsHelper.

cmsbuild · 2016-10-04T13:37:50Z

A new Pull Request was created by @rekovic for CMSSW_8_1_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-04T13:37:53Z

please test

cmsbuild · 2016-10-04T13:38:03Z

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

cmsbuild · 2016-10-04T15:37:15Z

+1
Tested at: 0f9c3fa
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16088/15519/summary.html

cmsbuild · 2016-10-04T17:15:21Z

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

rekovic · 2016-10-04T19:37:45Z

+1

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

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

davidlange6 · 2016-10-04T19:45:53Z

L1Trigger/L1TMuonBarrel/src/L1TMuonBarrelParamsHelper.cc

@@ -36,7 +36,7 @@ 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(9);
+	std::vector<int> pta_threshold(6); pta_threshold.reserve(10);


@rekovic - maybe you mean resize? reserve doesn't do too much here...

Yes it should be resiz, or better just declare with the correct size

@davidlange6 No, I meant reserve, as it was originally used by the developer, as the intended estimate of the capacity of the vector is known = 10.
This would only allocate memory, but leave vector elements uninitialized. They are filled
later on in the loop. But I guess either one reserve() and resize() would work.

In any case, there were no more "Invalid writes" from BMTF as originally reported by valgrind.

@ Yes, declaring vector initially with the correct size should also work. If preferred can make this change. Please advise.

that is not quite what reserve does - if you want a vector with 10 elements, you want resize(10) or better just std::vector pta_threshold(10) - otherwise you are relying on luck that push_back or reserve (etc etc) are never called on this vector.

OK. Changed now to declare vector with the correct size initially.

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

+1

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

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

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

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_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-06T09:32:25Z

+1
Tested at: db64d9a
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16088/15558/summary.html

cmsbuild · 2016-10-06T11:07:41Z

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

cmsbuild · 2016-10-06T11:31:52Z

+1
Tested at: 8a75953
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16088/15561/summary.html

cmsbuild · 2016-10-06T13:25:18Z

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

davidlange6 · 2016-10-06T23:10:13Z

+1

Dr15Jones · 2016-10-07T19:42:22Z

This appears to be causing a crash in the threaded and non-threaded IBs
https://cmssdt.cern.ch/SDT/cgi-bin/buildlogs/slc6_amd64_gcc530/CMSSW_8_1_NONTHREADED_X_2016-10-07-1100/pyRelValMatrixLogs/run/136.7321_RunZeroBias2016BnewL1repack+RunZeroBias2016BnewL1repack+HLTDR2newL1repack_2016+RECODR2newL1repack_2016reHLT_HIPM+HARVESTDR2/step3_RunZeroBias2016BnewL1repack+RunZeroBias2016BnewL1repack+HLTDR2newL1repack_2016+RECODR2newL1repack_2016reHLT_HIPM+HARVESTDR2.log

Dr15Jones · 2016-10-07T19:56:25Z

The crash is becuase the XmlConfigReader declares a destructor but no copy-constructor. The XmlConfigReader is held by value as a member data to TrigSystem, which is held by value as member data to L1TMuonBarrelParamsHelper which in turn is copied here
https://github.com/cms-l1t-offline/cmssw/blob/8a75953b6f0cdc8a12fc7a5f8e85718c180ba548/L1Trigger/L1TMuonBarrel/plugins/L1TMuonBarrelParamsESProducer.cc#L705

The eventually copied XmlConfigReader and the original one held by the ESProducer both share the same pointer to parser_. The copy in the EventSetup gets deleted first so deletes parser_. Then the one in the ESProducer gets delete, which does a double delete and crashes the program.

One should never write a destructor without also writing (or doing 'delete') a copy constructor and an operator=.

rekovic · 2016-10-10T07:59:35Z

Hi @Dr15Jones, I agree, the Rule of Three should always be followed.

However, it is a very strange thing that this crash reported in Barrel Muon ESProducer which uses "XmlConfigReader" from L1TCommon did not exist prior to the changes in the this PR. This PR had changes in the "XMLConfigProducer" from L1TMuonOverlap. Note that these are two different ConfigProducers - different names - XML vs Xml. In this PR a destructor to "XMLConfigProducer" was introduced, which also has a a call to XMLPlatformUtils::Terminate();.

According to this, having multiple calls to XMLPlatformUtils::Initilize() and XMLPlatformUtils::Terminate(), should not be a problem.

However, removing of XMLPlatformUtils::Terminate(); mitigates the crash, #16159.
Maybe @ianna can comment here.

rekovic added 2 commits October 4, 2016 15:14

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

93e8b7b

…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…

0f9c3fa

… in L1TMuonBarrelParamsHelper.

cmsbuild added this to the Next CMSSW_8_1_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

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

davidlange6 reviewed Oct 4, 2016

View reviewed changes

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

db64d9a

cmsbuild removed comparison-available fully-signed l1-approved labels Oct 6, 2016

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 comparison-available and removed comparison-pending labels Oct 6, 2016

cmsbuild added comparison-pending tests-approved and removed comparison-available 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 6, 2016

cmsbuild merged commit 5cbbe32 into cms-sw:CMSSW_8_1_X Oct 6, 2016

rekovic mentioned this pull request Oct 10, 2016

81x L1T - TEST - fix crash at endJob in test workflow 136.7321 #16159

Closed

rekovic mentioned this pull request Oct 11, 2016

Pr81x l1t XmlConigReader with Rule of Three #16179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr81x l1t memory fixes #16088

Pr81x l1t memory fixes #16088

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016 •

edited

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

davidlange6 Oct 4, 2016

davidlange6 Oct 5, 2016

rekovic Oct 5, 2016

rekovic Oct 5, 2016

davidlange6 Oct 6, 2016

rekovic 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

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

davidlange6 commented Oct 6, 2016

Dr15Jones commented Oct 7, 2016

Dr15Jones commented Oct 7, 2016

rekovic commented Oct 10, 2016

Pr81x l1t memory fixes #16088

Pr81x l1t memory fixes #16088

Conversation

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016 • edited

cmsbuild commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

rekovic commented Oct 4, 2016

cmsbuild commented Oct 4, 2016

davidlange6 Oct 4, 2016

Choose a reason for hiding this comment

davidlange6 Oct 5, 2016

Choose a reason for hiding this comment

rekovic Oct 5, 2016

Choose a reason for hiding this comment

rekovic Oct 5, 2016

Choose a reason for hiding this comment

davidlange6 Oct 6, 2016

Choose a reason for hiding this comment

rekovic Oct 6, 2016

Choose a reason for hiding this comment

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

cmsbuild commented Oct 6, 2016

cmsbuild commented Oct 6, 2016

davidlange6 commented Oct 6, 2016

Dr15Jones commented Oct 7, 2016

Dr15Jones commented Oct 7, 2016

rekovic commented Oct 10, 2016

cmsbuild commented Oct 4, 2016 •

edited

cmsbuild commented Oct 6, 2016 •

edited