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

KfComponentsHolder cleanup #4447

Merged
10 commits merged into from Jul 8, 2014
Merged

KfComponentsHolder cleanup #4447

10 commits merged into from Jul 8, 2014

Conversation

VinInn
Copy link
Contributor

@VinInn VinInn commented Jun 30, 2014

Cleanup of KfComponentsHolder.
It became a bit more urgent due to changes introduced by thread-safety.
Restore the timing performance of the Chi2MeasurementEstimator (still pretty deceiving though).

No regression expected, no regression observed

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @VinInn (Vincenzo Innocente) for CMSSW_7_2_X.

KfComponentsHolder cleanup

It involves the following packages:

Alignment/ReferenceTrajectories
DataFormats/Math
DataFormats/TrackerRecHit2D
DataFormats/TrackingRecHit
TrackingTools/GsfTracking
TrackingTools/KalmanUpdators

@nclopezo, @cmsbuild, @diguida, @rcastello, @StoyanStoynev, @slava77, @Degano can you please review it and eventually sign? Thanks.
@ghellwig, @frmeier, @GiacomoSguazzoni, @rovere, @gpetruc, @cerati, @mmusich, @venturia 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.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

#ifdef Debug_KfComponentsHolder
size_ = 0;
size_ == 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, should be as before "size_ = 0;".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops...
anyhow it compiles only for debug, so does not affect testing and production,
will be fixed

@StoyanStoynev
Copy link
Contributor

+1
Tested 2f8b670 on top of CMSSW_7_2_X_2014-06-27-0200.
Used extended runMatrix tests. No regressions observed and no other issues. Within few %, I see no difference in timing with wf 202 (ttbar with PU) and there is upto10 % positive effect on tracking modules in wf 16 and less clear in 17 (high and lower pt electron guns). For instance:
wf 202 (200 events):
electronGsfTracks 190.725 ms/ev -> 175.76 ms/ev
conversionTrackCandidates 98.4586 ms/ev -> 93.1699 ms/ev
(references: clusterSummaryProducer 63.7899 ms/ev -> 57.6302 ms/ev; siPixelClusterShapeCache 2.57173 ms/ev -> 2.3351 ms/ev)

wf 16 (2k events):
electronGsfTracks 744.966 ms/ev -> 631.855 ms/ev
conversionTrackCandidates 2179.84 ms/ev -> 1742.92 ms/ev
(references: clusterSummaryProducer 13.3835 ms/ev -> 11.568 ms/ev; siPixelClusters 17.4552 ms/ev -> 15.0882 ms/ev)

wf 17 (2k events):
electronGsfTracks 690.541 ms/ev -> 581.271 ms/ev
conversionTrackCandidates 1062.49 ms/ev -> 829.525 ms/ev
(references: clusterSummaryProducer 6.20514 ms/ev -> 5.02205 ms/ev; siPixelClusters 13.0698 ms/ev -> 9.72796 ms/ev; others also point to ~20% diff)

The bulk of the code changes consists of code restructuring. The non-trivial part deals with time optimizations (including usage of different objects).

@StoyanStoynev
Copy link
Contributor

... and a typo in the code should be fixed (no effect on default running).

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 4, 2014

@StoyanStoynev
Copy link
Contributor

Working on it. I'll compare the main features I observed with a previous commit using wf 202.
Jenkins are fine EXCEPT the many new warnings (compared with the previous tests) in the Unit tests.
These are probably not from this PR but one of the warnings (type) I mentioned in #4435:

%MSG-w TwoTrackMinimumDistance: PFTauPrimaryVertexProducer:hpsPFTauPrimaryVertexProducerPFlow

The others are for CondDB.

@StoyanStoynev
Copy link
Contributor

+1
Retested: 63ce78b on top of CMSSW_7_2_X_2014-07-04-0200. Performed extended tests with wf 202 and 16 (as before).
No issues, see the review above.
This time the machine was overloaded most of the time (that is why it took more time to complete as well) - the timing tests are less reliable but don't suggest excessive behavior just more jitter.
The code is mostly unchanged from the previously tested version though it is not identical (there are also changes to resolve a conflict, see above).

@diguida
Copy link
Contributor

diguida commented Jul 7, 2014

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 8, 2014

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine).

ghost pushed a commit that referenced this pull request Jul 8, 2014
KfComponentsHolder cleanup
@ghost ghost merged commit ea514a6 into cms-sw:CMSSW_7_2_X Jul 8, 2014
@VinInn VinInn deleted the KUcleanup branch July 13, 2016 13:47
This pull request was closed.
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

4 participants