[Insight-developers] Re: Threaded dilate/erode

Mon Nov 21 08:05:37 EST 2005

On Mon, 21 Nov 2005 13:21:23 +0100, SCHMID, Jerome  
<jeromeschmid at surgery.cuhk.edu.hk> wrote:

> Hi Gaetan,
>
> Could you please tell me what are your modifications, so I can see if it  
> is more
> thread-safe?

I have just modified the code to dilate the background instead of the  
foreground, so there is no reason to be more thread safe.
I have also do some changes to get the right result with a smaller  
requested region than the largest available one.
The code is available in ITK CVS or at  
http://voxel.jouy.inra.fr/darcs/itk/Insight/Code/BasicFilters/itkBinaryErodeImageFilter.txx

> Moreover have you tested with _non centered_ structured element?  
> Typically a
> ring structuring element. This ensures that the filter works with all  
> kind of SE.

I haven't tested that. I thought you have done it in your test with only  
one point in the structuring element...

>
> Moreover if the code is non-thread safe from an implementation viewpoint  
> but it
> works by running tests, I am not sure if we can still trust it. Maybe a  
> specific
> pgrm or conditions may create some errors....

You're right.
I need to be sure my code runs on several processors when I run tests, but  
for now, the tests I'm running on my own filters are showing that the  
performances are not really better with several threads, so have a single  
threaded filter seems to not be a real problem...

>
> Best Regards,
>
> Jerome Schmid
>
> -----------------------------------
> Jerome SCHMID
> Project Manager/ Engineer
> Augmented and Virtual Reality
> MIS Centre
> Prince of Wales Hospital
> Chinese University Of Hong-Kong
> -----------------------------------
>
>
>
> -----Original Message-----
> From: Gaetan Lehmann [mailto:gaetan.lehmann at jouy.inra.fr]
> Sent: Mon 11/21/2005 6:21 PM
> To: SCHMID, Jerome; Miller, James V (Research); Insight-developers  
> (E-mail)
> Subject: Re: [Insight-developers] Re: Threaded dilate/erode
>
>
> I have just wrote a small test to verify everything works with several
> threads.
> It is available at
> http://voxel.jouy.inra.fr/darcs/contrib-itk/binaryErodeDilate/
>
> The test dilate an image with a kernel of radius 20 to make a large
> overlaping region for each thread, and impose to run 100 threads. The
> result is compared with the output of the same filter with just 1 thread.
> I have run the test 100 fold without any failure.
> Everything seems to work at it should :-)
>
> To compare, when I had problem in my filters, with 10 threads and a  
> kernel
> of radius 5, I had errors for nearly each run.
>
> On Mon, 21 Nov 2005 10:31:26 +0100, Gaetan Lehmann
> <gaetan.lehmann at jouy.inra.fr> wrote:
>
>>
>> It should be easy to verify if there is a problem by writing a small
>> test where the number of thread is set to a high value (10 for example).
>> The filter I'm write had some problem with threads and this kind of test
>> show them very clearly.
>>
>> I modified the threaded version of Jerome to implement the erode filter
>> and to solve the problem with the requested region, but have not tested
>> it with several threads. If there is really a problem, we should go back
>> to the non threaded version, but we need to be sure the bug with the
>> requested region is fixed.
>>
>> Gaetan
>>
>>
>> On Mon, 21 Nov 2005 05:14:35 +0100, SCHMID, Jerome
>> <jeromeschmid at surgery.cuhk.edu.hk> wrote:
>>
>>> Miller, James V (Research) wrote:
>>>
>>>> Jerome, Gaetan,
>>>>
>>>> We have integrated the threaded versions of the binary dilate/erode
>>>> into the
>>> ITK repository. The use of the ProgressReporter needed to be changed
>>> slightly in
>>> the multithreaded versions to be thread safe. One of the arguments to
>>> the
>>> ProgressReporter is the threadId. Thread 0 is only one that outputs
>>> information
>>> by the ProgressReporter. This argument was set to 0 for all threads,
>>> causing
>>> some intermittent errors.
>>>>
>>> Hi Jim,
>>>
>>> Glad to see that you have interest on the filter implementation. BTW,
>>> just by
>>> curiosity, what is the effect of :
>>> "(void)ThreadId;" in the .txx code?
>>>
>>>>
>>>> I question whether stage 3 is really thread safe. It looks like this
>>>> loop can
>>> write to pixels that are outside the outputRegionForThread. This could
>>> result in
>>> two threads writing to the same memory location at the same time. This
>>> could
>>> result in corrupted memory and in some cases crashes. Can you verify
>>> whether or
>>> not stage 3 could write to pixels that are outside the
>>> outputRegionForThread.
>>>>
>>>
>>> Well I have a deeper look and it doesnt't seem thread-safe... However I
>>> am not
>>> an expert in iterators/multi-threading so let's see the hazardous
>>> points:
>>>
>>> // Paint SE --> "( BORDER(X) (+) B )" Part:
>>>
>>> // the dangerous iterator ^_^
>>> NeighborhoodIterator<OutputImageType> onit;
>>> onit = NeighborhoodIterator<OutputImageType>( kernel.GetRadius(),
>>> output,
>>> outputRegionForThread );
>>>
>>> ....
>>>
>>> // the devil part
>>> bool bIsInBound;
>>> for( itIndex = indexDifferenceSet.begin(); itIndex !=
>>> indexDifferenceSet.end();
>>> ++itIndex )
>>> {
>>> onit.SetPixel( *itIndex, foregroundValue, bIsInBound );
>>> }
>>>
>>> So the pb is: the neighborhoodIterator will write in is "neighboor
>>> band" that
>>> will overlap the close region of an other thread. The pb is that we
>>> need this
>>> writing if we want to have a correct result so it cannot be supressed.
>>>
>>>
>>> The last part doesn't cause pb because the output iterator is in the
>>> requestedRegionFroThread and the input iterator is in the
>>> requestedRegionFroThread padded but in READ mode, which should not
>>> cause pb, isn'it?
>>>
>>> So How to deal with the previous pb?
>>>
>>> Well I don't know exactly, I am not a thread expert...:-( But I think a
>>> mutex/barrier
>>> lock may become necessary only on the pixels belonging to the
>>> neighborhood that
>>> are outside the requested region of the iterator. This may require a  
>>> new
>>> iterator writing...
>>>
>>> Another solution, maybe not so easy to implement, would be to record
>>> these
>>> pixels to be writing and problematic and to store them in a structure
>>> without
>>> performing the writing. These pixels can be identified before the
>>> writing. Then,
>>> if itk offers this possibility, write these pixels in a *non* threaded
>>> way (AfterGenerateThreadedData()
>>> function or similar).
>>>
>>>
>>>>
>>>> Minor point: ITK style guide requires that TABS be replaced with
>>>> spaces. It
>>> would make integrating contributed code into ITK smoother if your
>>> submissions
>>> used spaces for indentation instead of TABS.
>>>>
>>>
>>> Yes you already told me but the threaded code was submitted a long time
>>> ago...:-)
>>> BTW I do not submit often !
>>>
>>> Tell me your thinking and we can try to find the best way. BTW I am
>>> surprised
>>> that there are no filter in itk with the same problematic...
>>>
>>> Best Regards,
>>>
>>> Jerome Schmid
>>>
>>> -----------------------------------
>>> Jerome SCHMID
>>> Project Manager/ Engineer
>>> Augmented and Virtual Reality
>>> MIS Centre
>>> Prince of Wales Hospital
>>> Chinese University Of Hong-Kong
>>> -----------------------------------
>>>
>>
>>
>>
>
>
>
> --
> 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
>
>

-- 
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