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

Fix thread-safety problems with beam spot fitting #18370

Merged
merged 1 commit into from Apr 18, 2017

Conversation

Dr15Jones
Copy link
Contributor

Changed how TH1::Fit was begin used to avoid thread-safety problems

  • forced ROOT to not use a global instance of Minuit
  • used the 'N' option of fitting to avoid adding the function to the
    global list of functions (since that list is not properly protected
    from manipulations from several threads simultaneously).

Changed how TH1::Fit was begin used to avoid thread-safety problems
- forced ROOT to not use a global instance of Minuit
- used the 'N' option of fitting to avoid adding the function to the
 global list of functions (since that list is not properly protected
 from manipulations from several threads simultaneously).
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

RecoVertex/BeamSpotProducer

@perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @mmusich, @davidlange6 can you please review it and eventually sign? Thanks.
@ghellwig, @makortel, @felicepantaleo, @GiacomoSguazzoni, @tocheng, @VinInn, @rovere, @ebrondol, @dgulhan this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 17, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19196/console Started: 2017/04/17 03:44

@Dr15Jones
Copy link
Contributor Author

@davidlange6 this should fix all or at least most of the crashes in the recent 9_1_X.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • You potentially added 28 lines to the logs
  • Reco comparison results: 1643 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1819442
  • DQMHistoTests: Total failures: 38209
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 1781059
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@arunhep
Copy link
Contributor

arunhep commented Apr 17, 2017

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 17, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19199/console Started: 2017/04/17 15:59

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 18, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19214/console Started: 2017/04/18 15:21

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Martin-Grunewald
Copy link
Contributor

FWIW, this PR solves the recent problems in the HLT Integration tests, which were of these types:

----- Begin Fatal Exception 18-Apr-2017 07:09:37 CEST-----------------------
An exception of category 'FatalRootError' occurred while
   [0] Processing  stream end LuminosityBlock run: 1 luminosityBlock: 84
stream: 0
   [1] Calling method for module AlcaBeamMonitor/'AlcaBeamMonitor'
   Additional Info:
      [a] Fatal Root Error: @SUB=TF1::AddToGlobalList
Function is supposed to be in the global list but it is not present

----- End Fatal Exception -------------------------------------------------
----- Begin Fatal Exception 18-Apr-2017 07:09:37 CEST-----------------------
An exception of category 'FatalRootError' occurred while
   [0] Processing  stream end LuminosityBlock run: 1 luminosityBlock: 84
stream: 2
   [1] Calling method for module AlcaBeamMonitor/'AlcaBeamMonitor'
   Additional Info:
      [a] Fatal Root Error: @SUB=TF1::AddToGlobalList
Function is supposed to be in the global list but it is not present

----- End Fatal Exception -------------------------------------------------

@@ -16,6 +16,7 @@
<use name="clhep"/>
<use name="rootcore"/>
<use name="rootminuit2"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this obsolete now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add rootminuit dependency to get the symbol for TMinuitMinimizer. I didn't check to see if rootminuit2 can go away since I'm not certain why it was there to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested and removing <use name="rootminuit2"/> does not appear to cause any adverse effects.
@davidlange6 do you want to me to update the pull request with this removed? Or do you want that as a seperate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separately is good. I'll merge this to restore the ibs...

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1744 differences found in the comparisons
  • DQMHistoTests: Total files compared: 23
  • DQMHistoTests: Total histograms compared: 1819408
  • DQMHistoTests: Total failures: 35631
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 1783604
  • DQMHistoTests: Total skipped: 173
  • DQMHistoTests: Total Missing objects: 0
  • Checked 94 log files, 14 edm output root files, 23 DQM output files

@davidlange6 davidlange6 merged commit 6ffec43 into cms-sw:master Apr 18, 2017
@davidlange6
Copy link
Contributor

PLEase add any further comments for later review.

@Dr15Jones
Copy link
Contributor Author

#18392 removed the unneeded dependency.

@arunhep
Copy link
Contributor

arunhep commented Apr 18, 2017

+1

@slava77
Copy link
Contributor

slava77 commented Apr 18, 2017

+1

for #18370 a26f8b9

  • post-merge signoff. Looks OK.
  • it looks like earlier thread-related changes in the framework should have been tested in multi-thread environment first before breaking the IBs.

@cmsbuild
Copy link
Contributor

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 requires discussion in the ORP meeting before it's merged. @Muzaffar, @davidlange6, @smuzaffar

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

6 participants