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

Thread safe updates pixel DQM - updated digi no longer accesses null pointers #3836

Merged
merged 1 commit into from
May 13, 2014

Conversation

leggat
Copy link
Contributor

@leggat leggat commented May 13, 2014

Thread safe and thread_unsafe updates, DigiSource updated all in one commit for pixel DQM.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @leggat for CMSSW_7_1_X.

Thread safe updates pixel DQM - updated digi no longer accesses null pointers

It involves the following packages:

DQM/SiPixelMonitorCluster
DQM/SiPixelMonitorDigi
DQM/SiPixelMonitorRawData
DQM/SiPixelMonitorRecHit
DQM/SiPixelMonitorTrack

@ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks.
@threus 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.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@deguio
Copy link
Contributor

deguio commented May 13, 2014

hello @leggat
thanks for this. the history of the commits is lost and I cannot disentangle what you did exactly. is this PR build on top of what I have done? what was the problem exactly?

thanks,
F.

@leggat
Copy link
Contributor Author

leggat commented May 13, 2014

hi @deguio
Ah, sorry. Yes, it is built on the 3781 PR . The only differences are in SiPixelMonitorDigi/src/SiPixelDigiSource.cc - on lines 746 and 244 an additional requirement of twoDimOnlyLayDisk being true is required before accessing the monitor elements that were causing the seg faults. They are only booked if this is true, but the SiPixelDigiModule code apparently didn't realise it hadn't booked them and was trying to access random memory.
Cheers,
Duncan

@deguio
Copy link
Contributor

deguio commented May 13, 2014

is this changing any of the histograms content or should we expect an exact match with pre7?
thanks. waiting for the tests to be completed. hopefully I will be able to approve before pre8 closes.

ciao,
F.

@cmsbuild
Copy link
Contributor

@deguio
Copy link
Contributor

deguio commented May 13, 2014

+1
@leggat since you had to reorganise the code a little bit (thanks for this) please keep in touch with the validators to make sure that everything is behaving the right way.
thanks,
F.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

ktf added a commit that referenced this pull request May 13, 2014
Thread safe updates pixel DQM - updated digi no longer accesses null pointers
@ktf ktf merged commit f971fa9 into cms-sw:CMSSW_7_1_X May 13, 2014
@cmsbuild
Copy link
Contributor

@wmtan
Copy link
Contributor

wmtan commented May 15, 2014

To leggat and other DQM people: This commit to DQM/SiPixelMonitorDigi causes massive relval failures due to segfaults in the ROOT 6 IB. The segfaults do not occur in the same tests every time.
Since ROOT6 is not a high priority for you at this time, I don't expect you to do anything about this at the moment, but, If you do intend to fix this, let me know ASAP. Otherwise, I will attempt to back the
DQM/SiPixelMonitorDigi changes out of the ROOT6 IB for now, and this can be dealt with later.

@wmtan
Copy link
Contributor

wmtan commented May 15, 2014

To leggat and other DQM people: After a short discussion with Chris Jones, I've decided to try and fix the problem seen in the ROOT6 IB rather than backing out the DQM/SiPixelMonitorDigi portion of this pull request.

@deguio
Copy link
Contributor

deguio commented May 16, 2014

ktf added a commit to ktf/cmssw that referenced this pull request May 16, 2014
This reverts commit f971fa9, reversing
changes made to 406887d.
ktf added a commit that referenced this pull request May 16, 2014
Revert "Merge pull request #3836 from leggat/threadSafe"
@wmtan
Copy link
Contributor

wmtan commented May 16, 2014

FWIW, in the ROOT6 IB I could not reproduce any of the many segfaults seen in the ROOT6 IB when I ran the relvas myself on cmsdev04. All tests pass. The crashes are not reproducible, at least in the ROOT6 IB.

@ktf
Copy link
Contributor

ktf commented May 16, 2014

As you might have seen above, I reverted the merge of this pull request.

@leggat
Copy link
Contributor Author

leggat commented May 23, 2014

In case you haven't seen it, I fixed the raw data error in #3952 which should supersede this.

davidlange6 pushed a commit to davidlange6/cmssw that referenced this pull request Mar 21, 2018
add -Wno-unknown-warning-option for clang compiler
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.

5 participants