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 changes for DTTrig class #910

Merged
merged 3 commits into from Oct 6, 2013
Merged

Thread safe changes for DTTrig class #910

merged 3 commits into from Oct 6, 2013

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented Sep 24, 2013

No description provided.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @vkuznet (Valentin Kuznetsov) for CMSSW_7_0_X.

Thread safe changes for DTTrig class

It involves the following packages:

CondFormats/DTObjects

@smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please review it and eventually sign? Thanks.
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.
@ktf you are the release manager for this.

@vkuznet
Copy link
Contributor Author

vkuznet commented Sep 24, 2013

@Dr15Jones please review

pBuf = const_cast<DTBufferTree<int,int>**>( &dBuf );
*pBuf = new DTBufferTree<int,int>;
//
#if !defined(__CINT__) && !defined(__MAKECINT__) && !defined(__REFLEX__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the #if for ROOT here? ROOT's parser (Cint) should never have to parse this file. If it is then we should look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I removed these statements and code compiles just fine.

On Sep 24, 2013, at ,Sep 24, 12:26 PM, Chris Jones wrote:

In CondFormats/DTObjects/src/DTTtrig.cc:

@@ -396,9 +395,12 @@ void DTTtrig::cacheMap() const {
// std::string mName = mapName();
// DTBufferTree<int,int>* dBuf =
// DTDataBuffer<int,int>::openBuffer( mName );

  • DTBufferTree<int,int>** pBuf;
  • pBuf = const_cast<DTBufferTree<int,int>**>( &dBuf );
  • *pBuf = new DTBufferTree<int,int>;
    +//
    +#if !defined(CINT) && !defined(MAKECINT) && !defined(REFLEX)
    Why the #if for ROOT here? ROOT's parser (Cint) should never have to parse this file. If it is then we should look into it.


Reply to this email directly or view it on GitHub.

@cmsbuild
Copy link
Contributor

Pull request #910 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.


//atomically try to swap this to become dBuf
DTBufferTree<int,int>* expect = nullptr;
bool exchanged = dBuf.compare_exchange_strong(expect, pBuf);
Copy link
Contributor

Choose a reason for hiding this comment

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

should use std::memory_order_acq_rel

@Dr15Jones
Copy link
Contributor

All (*dBuf). should become (*dBuf.load(std::memory_order_acquire)).

@ktf
Copy link
Contributor

ktf commented Sep 25, 2013

And you complained about my comma operator?!?!?!?! ;-)

@cmsbuild
Copy link
Contributor

Pull request #910 was updated. @smuzaffar, @apfeiffer1, @nclopezo, @demattia, @rcastello, @ggovi can you please check and sign again.

@Dr15Jones
Copy link
Contributor

Looks good to me

@apfeiffer1
Copy link
Contributor

+1

1 similar comment
@rcastello
Copy link

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 6, 2013

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

ktf added a commit that referenced this pull request Oct 6, 2013
Thread safety fixes -- Thread safe changes for DTTrig class
@ktf ktf merged commit 6fb77a7 into cms-sw:CMSSW_7_0_X Oct 6, 2013
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

7 participants