cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.09k stars 4.33k forks source link

Igprof analysis of 11_1_0_p3_ROOT618 : CSCMotherboard::CSCMotherboard #30745

Open tommasoboccali opened 4 years ago

tommasoboccali commented 4 years ago

Dear all, an igprof analysis of L1+RECO here shows a large allocation (~ 220 MB) in

CSCMotherboard::CSCMotherboard(unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, edm::ParameterSet const&)

direct link to the report HERE

Can you please have a look whether this is expected / correct? cheers

cmsbuild commented 4 years ago

A new Issue was created by @tommasoboccali Tommaso Boccali.

@Dr15Jones, @silviodonato, @dpiparo, @smuzaffar, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel commented 4 years ago

assign l1

cmsbuild commented 4 years ago

New categories assigned: l1

@benkrikler,@rekovic you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel commented 4 years ago

It looks tome that the main culprit is https://github.com/cms-sw/cmssw/blob/0393b55ee8db053139999fc635a7bbc44c39ee3a/L1Trigger/CSCTriggerPrimitives/interface/CSCCathodeLCTProcessor.h#L201 coming via https://github.com/cms-sw/cmssw/blob/0393b55ee8db053139999fc635a7bbc44c39ee3a/L1Trigger/CSCTriggerPrimitives/src/CSCMotherboard.cc#L50

The PulseArray is defined as https://github.com/cms-sw/cmssw/blob/0393b55ee8db053139999fc635a7bbc44c39ee3a/L1Trigger/CSCTriggerPrimitives/interface/CSCCathodeLCTProcessor.h#L101 and NUM_LAYERS is 6 and NUM_HALF_STRIPS_7CFEBS is 225. That makes 6*225*99*4 bytes = 523 kB. GDB gave me 540 hits in the CSCMotherboard constructor (in the workflow mentioned in https://github.com/cms-sw/cmssw/issues/30742#issuecomment-659341333), which makes the total 540*523 kB = 275 MB (this doesn't exactly match to the IgProf report though).

FYI @dildick

dildick commented 4 years ago

I think that the member PulseArray hitsCLCT[99]; was removed in a recent CMSSW_11_2_X version. See https://github.com/cms-sw/cmssw/pull/30103. I need to keep track of all the hits for each CLCT candidate for each BX. See https://github.com/cms-sw/cmssw/blob/master/L1Trigger/CSCTriggerPrimitives/src/CSCCathodeLCTProcessor.cc#L573-L575. This container is filled with hits for each CLCT candidate https://github.com/cms-sw/cmssw/blob/master/L1Trigger/CSCTriggerPrimitives/src/CSCCathodeLCTProcessor.cc#L892

tommasoboccali commented 4 years ago

Dear Sven, I am not sure I get this comment. Is this for the memory reduction, or a real bug? If the latter, does it need to be ported also in 11_1_X?

(by the way, I cannot even see the comment here in github, I only got it from a github email... strange)

On Tue, Jul 28, 2020 at 6:51 PM Sven Dildick notifications@github.com wrote:

This condition https://github.com/cms-sw/cmssw/blob/master/L1Trigger/CSCTriggerPrimitives/src/CSCTriggerPrimitivesBuilder.cc#L340 should be

else if (isSLHC and ring == 1 and ((stat == 2 and runME21Up and !runME21ILT) || (stat == 3 and runME31Up) || (stat == 4 and runME41Up_))) {

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/issues/30745#issuecomment-665153031, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7HMYOGZY4ML6HGHJRKK7DR536WTANCNFSM4O3ZF2JA .

-- Tommaso Boccali INFN Pisa

dildick commented 4 years ago

The comment posted earlier is only relevant for a different issue.

After removing PulseArray hitsCLCT[99];, is the large allocation still there?

dildick commented 4 years ago

@tommasoboccali Should have a look at newer versions of the CSC trigger if the situation has changed?

tommasoboccali commented 4 years ago

Ciao! the HLT production has started, so we are not in the critical path for that. Still, in the long run we will need to reduce the memory footprint of Phase-2 sw, so yes, a look would be appreciated 👍

thanks

tom