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

DD Generic Singleton Pattern #23147

Merged
merged 5 commits into from May 9, 2018
Merged

Conversation

ianna
Copy link
Contributor

@ianna ianna commented May 4, 2018

  • Simple to reuse the same implementation across all singletons use cases
  • More than a single singleton can be constructed for the same type
  • A singleton can be applied to any type which one can construct, not necessarily with a default constructor
  • Defer the singleton initialization until the first use
  • The singleton initialization is thread safe:
    • Only a single thread will initialize the singleton
    • All the threads will use the same initialized instance
  • Using the singleton is as fast as direct pointer access
  • Add unit test and a couple of tests for valgrind
$ valgrind --leak-check=full --show-leak-kinds=all ../../../test/slc6_amd64_gcc630/testSingleton
==16045== Memcheck, a memory error detector
==16045== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==16045== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==16045== Command: ../../../test/slc6_amd64_gcc630/testSingleton
==16045== 
Hello world
==16045== 
==16045== HEAP SUMMARY:
==16045==     in use at exit: 0 bytes in 0 blocks
==16045==   total heap usage: 2 allocs, 2 frees, 72,736 bytes allocated
==16045== 
==16045== All heap blocks were freed -- no leaks are possible
==16045== 
==16045== For counts of detected and suppressed errors, rerun with: -v
==16045== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

$ valgrind --leak-check=full --show-leak-kinds=all ../../../test/slc6_amd64_gcc630/testSingletonInit 
==2464== Memcheck, a memory error detector
==2464== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==2464== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==2464== Command: ../../../test/slc6_amd64_gcc630/testSingletonInit
==2464== 
Initialized
Hello
==2464== 
==2464== HEAP SUMMARY:
==2464==     in use at exit: 0 bytes in 0 blocks
==2464==   total heap usage: 2 allocs, 2 frees, 72,736 bytes allocated
==2464== 
==2464== All heap blocks were freed -- no leaks are possible
==2464== 
==2464== For counts of detected and suppressed errors, rerun with: -v
==2464== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

Note, test1 uses getInstance():

Package DetectorDescription/DDCMS: Running test testSingletonBenchmark
 
===== Test "testSingletonBenchmark" ====
Benchmark 'test1' took 0.000699 millis
Benchmark 'test2' took 0.001109 millis
Benchmark 'test3' took 0.001125 millis
Benchmark 'test4' took 0.000953 millis
Benchmark 'test5' took 0.000975 millis
Benchmark 'test6' took 0.001117 millis

---> test testSingletonBenchmark succeeded

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

DetectorDescription/DDCMS

@cmsbuild, @civanch, @Dr15Jones, @ianna, @mdhildreth can you please review it and eventually sign? Thanks.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@ianna
Copy link
Contributor Author

ianna commented May 4, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27806/console Started: 2018/05/04 17:22

@Dr15Jones
Copy link
Contributor

I believe a better way is to use a static getInstance() class method. The method would be written as

template<typename T>
T* getInstance() {
   static std::unique_ptr<T> s_instance{ std::make_unique<T>() };
   return s_instance.get();
}

This avoids the problem of having to declare the m_instance as well as __attribute__(( unused )) specification.

I would then change your class to hold the pointer, thereby avoiding the slight overhead of calling getInstance() on each use of DDSingleton.

DDSingleton() : m_instance(getInstance()) {}
...
T* m_instance;

@ianna
Copy link
Contributor Author

ianna commented May 4, 2018

@Dr15Jones - Thanks for looking into this! I just fancied using lambda expressions and the C++11 thread safety guarantee for function-local statics. I'll implement your suggestion and run the comparison tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 4, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2713580
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2713396
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

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

@ianna
Copy link
Contributor Author

ianna commented May 7, 2018

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/27832/console Started: 2018/05/07 17:39

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 30
  • DQMHistoTests: Total histograms compared: 2713614
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2713430
  • DQMHistoTests: Total skipped: 183
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 29 files compared)
  • Checked 124 log files, 14 edm output root files, 30 DQM output files

@ianna
Copy link
Contributor Author

ianna commented May 7, 2018

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented May 7, 2018

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

fabiocos commented May 9, 2018

+1

@cmsbuild cmsbuild merged commit 4388080 into cms-sw:master May 9, 2018
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