[Insight-developers] type of modification time

[Matt McCormick]

On Tue, Sep 25, 2012 at 8:01 AM,  <M.Staring at lumc.nl> wrote:
> Hi Matt,
>
> Thanks for the feedback. The two suggestions don't help in my case. Calling Update() one or more times has the same result, nothing happens. Also, I am using the function CreateElementAt() which really modifies the container, so no circumvention of Modified() there. But the feedback is well appreciated.
>
> ---
>
> Regarding the place of the ModifiedTimeType typedef, to put it in itkIntTypes or somewhere else: The function GetMTime is re-defined at many places throughout the toolkit. This means I would have to propagate the typedef to all these classes (typedef typename Superclass::ModifiedTimeType ModifiedTimeType).'
>
> In addition, the situation is actually worse. DataObject has a public function
> void SetPipelineMTime(ModifiedTimeType time)
> so it also needs this typedef.
> Derived classes of ProcessObject also use it.
> Object has virtual ModifiedTimeType GetMTime() const;
> also public.
> This means that all derived classes from Object have the public function GetMTime, so people can call
> someType mtime = derivedClass->GetMTime();
> So, they need to know that someType == ModifiedTimeType, which they should be able to get through
> typedef DerivedClassType:: ModifiedTimeType ModifiedTimeType;
> which means that we would have to copy the typedef to all classes in the toolkit.
>
> It is easier to add one typedef to itkIntTypes.
>
> What are your thoughts?

Hi Marius,

In this light, a typedef in itkIntTypes.h sounds reasonable to me.

Thanks,
Matt


>
> Regards, Marius
>
> -----Original Message-----
> From: Matt McCormick [mailto:matt.mccormick at kitware.com]
> Sent: maandag 24 september 2012 22:56
> To: Sean McBride
> Cc: Bradley Lowekamp; insight-developers at itk.org; Staring, M. (LKEB)
> Subject: Re: [Insight-developers] type of modification time
>
> Hi Marius,
>
> I encountered a similar issue when helping with the FARSIGHT project.
> In that case, the modified time would rollover in the context of ITK filters used in OpenMP threads.  When the rollover happened, the pipeline did not update correctly, which predictably caused an "region not available" type of exception.  The workaround in that case was simply call Update() again.  The code to do this:
>
>   https://farsight-svn.ee.uh.edu/repos/farsight/trunk/ftkCommon/ftkTimeStampOverflowSafeUpdate.h
>   https://farsight-svn.ee.uh.edu/repos/farsight/trunk/ftkCommon/ftkTimeStampOverflowSafeUpdate.hxx
>
> How the fix was tested:
>
>   https://farsight-svn.ee.uh.edu/repos/farsight/trunk/ftkCommon/itkRolloverOpenMPTest.cpp
>
> That may or may not help in your case.
>
>
> On a related tangent: yesterday I merged a patch submitted by Ho related to VectorContainer excessive calls to Modified().  There are const and non-const versions of methods in VectorContainer, and the compiler was not smart enough to pick the const version.  This resulted in excessive calls to Modified(), an associated performance hit, and progression towards the modified time rollover.  The problem was detected by examining valgrind/callgrind output, and the solution was to force the compiler to use the const version with a cast.  More information here:
>
>   http://review.source.kitware.com/#/c/7603/
>
>
> Whether or not those two approaches help with your case, the patch you are proposing is a good step forward.  More typedefs instead of hard-coded types are helpful.  As Brad L. remarked, it would be better if the typedef was a member of the itk::TimeStamp class since it is specific to this class.  As Sean remarked, the type of this typedef is going to depend on the platform, and we may get 64 bit support for at least some platforms and open the possibility for 64 bit modified time in the future.
>
> Thanks,
> Matt
>
> On Mon, Sep 24, 2012 at 5:42 PM, Sean McBride <sean at rogue-research.com> wrote:
>> On Mon, 24 Sep 2012 14:43:03 +0000, Matt McCormick said:
>>
>>>The type of the modified time is limited by the platform-specific
>>>functions to perform the atomic operation that increments it.  We are
>>>limited to the type used in the platform specific functions, and I do
>>>not think what you are proposing will work (although I would be happy
>>>to be proved wrong :-) ).
>>
>> Indeed, it may be tricky for that reason.
>>
>> Also, on OS X, the OSAtomicIncrement64() function is available on 64 bit PowerPC, but not 32 bit PowerPC... OSAtomicIncrement32 is available on both, which is what is used now.  We could always fall back to the slow mutex on PPC32 I guess.
>>
>> One portable thing that could be done is to use C++11's new atomic stuff, and fall back to the platform-specific code only if the compiler does not support C++11.  Few compilers support it yet, but it's more future-proof.
>>
>> Cheers,
>>
>> --
>> ____________________________________________________________
>> Sean McBride, B. Eng                 sean at rogue-research.com
>> Rogue Research                        www.rogue-research.com
>> Mac Software Developer              Montréal, Québec, Canada
>>
>>