Speed up the use of DDFilteredViews #17778
Conversation
Generalized the usefulness of DDFilteredViewAnalyzer by adding a few more options. This allowed it to be used for a wider range of testing and for profiling.
All code that used DD filtering only used the default DDLogOp::AND. Removing the OR option allows for a great deal of simplification.
All calls to DDSpecificsFilter.addCriteria used the default values for the arguments which had defaults. Therefore those options were removed and the logic only supports the old default values. In addition, only DDCompOp::equals and not_equals are actually used so the other choices were removed.
Created unit test for DDFilter to be used in conjunction with DDFilteredView.
Decreased the time filtering by avoiding making a temporary container through the call to DDLogicalPart::mergedSpecificsV. In tests with Upgrade SIM, the initialization time of OscarProducer was sped up by a 3x.
Removed the redundant DDCompOp::matches and DDCompOp::not_matches
The original DDSpecificsFilter is capable of doing many different kinds of queries which caused the code to be extremely complex and slow. The new DDFilters do one type of query and can be composed using the DDAndFilter.
From profiling the initialization time for OscarProducer it was found that many temporaries of type MuonDDDConstants were being produced. This was taking significant amounts of time. It is now possible to build just one MuonDDDConstants per stream.
The original implementation of DDFilteredView allowed for multiple DDFilters to be used. However, DDFilters are already composible so that functionality is unneeded and just complicates the design and slows the code. Changed the constructor of DDFilteredView to enforce that one and only one DDFilter is ever attached.
Rearranged the logic in the DDFilters to minimize the calls to DDCompareEqual since profiling showed this to be a very expensive call.
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages: Alignment/CocoaApplication @ghellwig, @civanch, @Dr15Jones, @arunhep, @ianna, @mdhildreth, @dmitrijus, @cmsbuild, @franzoni, @kpedro88, @cerminar, @mmusich, @vanbesien, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here #13028 |
|
please test |
|
The tests are being triggered in jenkins. |
|
@ianna I'd appreciate it if you'd do a careful code review of this pull request. |
|
@ianna is there any reason to keep DetectorDescription/RegressionTest/test/tutorial.cc? It looks like it only works if someone externally fills the singletons for a DDCompactView. |
| @@ -34,14 +34,10 @@ bool CSCGeometryParsFromDD::build( const DDCompactView* cview | |||
|
|
|||
| DDSpecificsFilter filter; | |||
There was a problem hiding this comment.
I should have changed this to DDSpecificsMatchesValueFilter.
|
As far as I can tell, all the comparisons are normal. |
|
@Dr15Jones - Yes, I'm just running some extra tests. |
|
+1 |
|
+1 |
|
For some reasons my local comparisons show differences for muon hits (nothing for calorimeters) but official comparison are OK. May this be due to some change in rotations? Another concern: there is a report about broken CMS code rules in this PR (?) - this is unusual. |
|
@civanch the coding rule violations are ones already in the IB. From this pull request the violations are: If you look at the most recent IB Q&A you'll see those same items: |
|
@civanch How can one reproduce the comparison problems you are seeing locally? |
|
@Dr15Jones , I run 600 MeanBias events at recent IB with 8 threads, then merge locally this PR and run again. Number of hits in CSC is not big for this statistics (~500) but the difference is visible. It may be a false alarm, because these hits may be due to few events. My little worry is due to expectation that nothing should be affected and sim history should not change at all. Probably, I have to rerun with 1 thread. In calorimeters number of hits is significant and distributions are really very similar. |
|
@civanch do you happen to have the exact cmsDriver command you used to generate the configuration? |
|
+1 |
|
@civanch David Dagenhart and I discussed this and one would expect the possibility of different results using multiple threads. We tie random number seeds to StreamIDs. However, when running different jobs using the same configuration with multiple threads, the streams in the two different jobs are likely to process different number of events and therefore use different random number seeds. As an example, say you are going to generate 8 events and use 4 streams. In the first job you might get the following stream to #events processed in another job you could get So between these two jobs, you'd have 6 events which saw the same random numbers and 2 events which saw different random numbers. |
|
@dmitrijus @vanbesien @cerminar @franzoni @mmusich @ghellwig @arunhep please review this pull request. |
|
+1 |
Profiling of multi-threaded GEN+SIM jobs showed that the initialization time in RunManagerMTWorker::initializeThread was severely impacting the total event throughput for jobs with large numbers of streams (>100) and only modest numbers of events per stream. Using 12 cores on a standard Xeon system under CMSSW_9_0_0_pre5 the call to initializeThread took 517 seconds. After all the changes in this pull request it now takes 51 seconds.
The changes were
-Replace DDSpecificsFilter with filters that are specifics to the needs of the query.
-Have DDFilteredView only take one DDFilter (no code used more than one every anyway).
-Test DDFilters with a standard unit test.