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

reduce creation of string objects and use char* instead of string #9678

Merged

Conversation

ngrenz
Copy link
Contributor

@ngrenz ngrenz commented Jun 19, 2015

PR of a work intended to optimize gradually the perfrormance of tracker validation code.
Outputs are supposed to be unchanged.
Any comments and suggestions are welcome -more optimizations will follow in the near future

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @ngrenz for CMSSW_7_5_X.

reduce creation of string objects and use char* instead of string

It involves the following packages:

SimTracker/TrackerHitAssociation

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @appeltel, @wmtford, @cerati, @threus, @dgulhan, @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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@ngrenz
Copy link
Contributor Author

ngrenz commented Jun 19, 2015

@boudoul FYI

@@ -99,7 +99,7 @@ void TrackerHitAssociator::makeMaps(const edm::Event& theEvent, const TrackerHit
// The collections are specified via ROUList in the configuration, and can
// be either crossing frames (e.g., mix/g4SimHitsTrackerHitsTIBLowTof)
// or just PSimHits (e.g., g4SimHits/TrackerHitsTIBLowTof)

const char* highTag = "HighTof";
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be even better to make this a const char * const. Then neither the character array nor the pointer would be allowed to change.

@makortel
Copy link
Contributor

@ngrenz Have you profiled the code?

@ngrenz
Copy link
Contributor Author

ngrenz commented Jun 19, 2015

@makortel I have profiled the same change in CMSSW_7_4_0 with Igprof but not in the version 7_5_X.
The result was that funktion makeMaps was 1.5 times faster with this change.

@civanch
Copy link
Contributor

civanch commented Jun 19, 2015

@ngrenz , I would agree with Matti, that this string comparison may be done before the loop lines 108-122.

@makortel
Copy link
Contributor

@ngrenz Very good, looking forward to your further optimizations.

@boudoul
Copy link
Contributor

boudoul commented Jun 22, 2015

thanks all for your comments
@ngrenz : could you please update your branch with the suggestion from @makortel and @Dr15Jones ?
Thanks

@cmsbuild
Copy link
Contributor

Pull request #9678 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

@ngrenz
Copy link
Contributor Author

ngrenz commented Jun 22, 2015

I had some problems with Igprof test of CMSSW_7_5_X and my simulation configuration, that is why I reconstruct the changes in 7_4_0 and profiled them with Igprof.
It is only a small simulation: 10 Events, AVE_75_BX_25ns

The results
original code:
http://test-ngrenz-igprof-result-presentation.web.cern.ch/test-ngrenz-igprof-result-presentation/cgi-bin/igprof-navigator/step3_2015-06-22_original/177

change string to char*:
http://test-ngrenz-igprof-result-presentation.web.cern.ch/test-ngrenz-igprof-result-presentation/cgi-bin/igprof-navigator/step3_2015-06-22_string/571

and change string comparison position:
http://test-ngrenz-igprof-result-presentation.web.cern.ch/test-ngrenz-igprof-result-presentation/cgi-bin/igprof-navigator/step3_2015-06-22_find/1234

and change last if position (and small variable changes):
http://test-ngrenz-igprof-result-presentation.web.cern.ch/test-ngrenz-igprof-result-presentation/cgi-bin/igprof-navigator/step3_2015-06-22_if/1145

The last result maybe slower because:

  • I have made a mistake
  • my reconstruction in 7_4_0 is not good
  • fluctuations of the program (the number of events was low)

@cmsbuild
Copy link
Contributor

Pull request #9678 was updated. @cmsbuild, @civanch, @mdhildreth can you please check and sign again.

@mdhildreth
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs once checked with relvals in the development release cycle of CMSSW (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Jun 25, 2015
reduce creation of string objects and use char* instead of string
@cmsbuild cmsbuild merged commit dd8b482 into cms-sw:CMSSW_7_5_X Jun 25, 2015
@ngrenz ngrenz deleted the CMSSW_7_5_X_ngrenz_string_makeMaps branch July 20, 2015 08:30
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

8 participants