[Insight-developers] LightObject race condition fix

[Gaëtan Lehmann]

Hi Luis,

Of course you're right, that's the best way to be sure that their is  
no performance impact.
The test is available at http://voxel.jouy.inra.fr/darcs/contrib-itk/ 
raceCondition/
It is pretty simple, but should be reviewed to be sure it is doing  
what it is supposed to do :

   it creates and destroys an itk::LightObject 1 million times.

without the fix: 0.365999s
with the fix: 0.365827s

There are some small variation from one run to the other, but  
globally, I can't see any performance changes.

Gaëtan


Le 6 août 07 à 15:39, Luis Ibanez a écrit :

>
> Hi Gaetan,
>
> We probably should setup a profiling test before committing
> these changes.
>
> It could be something as simple as creating and destroying
> any ITK object for 10,000 times, and measuring the total
> time using the itkTimesProbeCollectorBase.
>
> That will give us an idea of the variations in computation
> time, before and after introducing the changes.
>
>
>   Luis
>
>
> ----------------------
> Gaëtan Lehmann wrote:
>> Le 6 août 07 à 13:38, Bill Lorensen a écrit :
>>> Gaeten,
>>>
>>> Which changes did we decide on? We should be very careful here.  
>>> We  don't want any efficiency problems.
>>>
>> yes, that's a sensible peace of code - that's why I made the  
>> changes  as small as possible. More changes should be carrefully  
>> reviewed.
>> The changes are:
>> [glehmann at marvin Common]$ cvs diff -u
>> Index: itkLightObject.cxx
>> ===================================================================
>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.cxx,v
>> retrieving revision 1.34
>> diff -u -r1.34 itkLightObject.cxx
>> --- itkLightObject.cxx  4 Apr 2007 20:04:10 -0000       1.34
>> +++ itkLightObject.cxx  6 Aug 2007 12:07:57 -0000
>> @@ -150,12 +150,12 @@
>> ::UnRegister() const
>> {
>>    m_ReferenceCountLock.Lock();
>> -  m_ReferenceCount--;
>> +  int tmpReferenceCount = --m_ReferenceCount;
>>    m_ReferenceCountLock.Unlock();
>>    // ReferenceCount in now unlocked.  We may have a race condition
>>    // to delete the object.
>> -  if ( m_ReferenceCount <= 0)
>> +  if ( tmpReferenceCount <= 0)
>>      {
>>      delete this;
>>      }
>> Index: itkLightObject.h
>> ===================================================================
>> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.h,v
>> retrieving revision 1.33
>> diff -u -r1.33 itkLightObject.h
>> --- itkLightObject.h    6 Feb 2006 22:01:56 -0000       1.33
>> +++ itkLightObject.h    6 Aug 2007 12:07:57 -0000
>> @@ -114,7 +114,7 @@
>>    virtual void PrintTrailer(std::ostream& os, Indent indent) const;
>>    /** Number of uses of this object by other objects. */
>> -  mutable int m_ReferenceCount;
>> +  volatile mutable int m_ReferenceCount;
>>    /** Mutex lock to protect modification to the reference count */
>>    mutable SimpleFastMutexLock m_ReferenceCountLock;
>> Gaëtan
>>> Bill
>>>
>>> On 8/6/07, Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr> wrote:
>>> Le 12 juil. 07 à 00:31, Peter Cech a écrit :
>>>
>>> > On Wed, Jul 11, 2007 at 11:02:16 -0400, Karthik Krishnan wrote:
>>> >> On 7/11/07, Gaëtan Lehmann <gaetan.lehmann at jouy.inra.fr> wrote:
>>> >>>
>>> >>>
>>> >>> Le 11 juil. 07 à 15:33, Karthik Krishnan a écrit :
>>> >>>
>>> >>>> However I agree with Steve/Luis that access of the resource
>>> >>>> m_ReferenceCount be thread safe. One solution would be to use a
>>> >>>> scoped lock (unlocking is implemented in its destructor, so it
>>> >>>> automatically does that when it goes out of scope).
>>> >>>>
>>> >>>> The code would look like
>>> >>>>
>>> >>>> UnRegister()
>>> >>>> {
>>> >>>>   ScopedLock lock( m_ReferenceCountLock )
>>> >>>>   m_ReferenceCount--;
>>> >>>>   if ( m_ReferenceCount <= 0) { delete this;  }
>>> >>>> }
>>> >>>
>>> >>> Please correct if I'm wrong, but the mutex lock will be   
>>> destroyed by
>>> >>> the delete call before the destruction of the ScopedLock object,
>>> >>
>>> >>
>>> >> You're right.. How silly of me :) I'd suggest leaving it as is..
>>> >
>>> > It's past midnight here, so I'll refrain from trying to post any
>>> > solutions, but there is something relevant here:
>>> >
>>> > http://softwarecommunity.intel.com/isn/Community/en-US/forums/
>>> > thread/30225804.aspx
>>> >
>>> > Enjoy!
>>> >
>>>
>>> Hi,
>>>
>>> I realize I haven't put the changes in the cvs repository.
>>> Can I do that ?
>>>
>>> Would be great also to explore Peter's link. It seem quite complex
>>> though, and I didn't get enough time yet to look at it carefully...
>>>
>>> Gaëtan
>>>
>>>
>>> -- 
>>> Gaëtan Lehmann
>>> Biologie du Développement et de la Reproduction
>>> INRA de Jouy-en-Josas (France)
>>> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
>>> http://voxel.jouy.inra.fr
>>>
>>>
>>>
>>> _______________________________________________
>>> Insight-developers mailing list
>>> Insight-developers at itk.org
>>> http://www.itk.org/mailman/listinfo/insight-developers
>>>
>> -- 
>> Gaëtan Lehmann
>> Biologie du Développement et de la Reproduction
>> INRA de Jouy-en-Josas (France)
>> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
>> http://voxel.jouy.inra.fr
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers at itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers

--
Gaëtan Lehmann
Biologie du Développement et de la Reproduction
INRA de Jouy-en-Josas (France)
tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
http://voxel.jouy.inra.fr