[Insight-developers] type of modification time

[M.Staring at lumc.nl]

Hi Matt,

On further inspection it appeared your suggestion makes a lot of sense for our case as well. We were able to avoid the CreateElementAt() function, thereby avoiding a lot of the Modified()'s. There was still an overflow, so we had to search a bit further. Somewhere downstream ElementAt() was (!) called; hidden in external code. We are now implementing your work-around to use the const version of this function.

So, thank you very much for the feedback!
Marius

-----Original Message-----
From: insight-developers-bounces at itk.org [mailto:insight-developers-bounces at itk.org] On Behalf Of M.Staring at lumc.nl
Sent: dinsdag 25 september 2012 10:02
To: matt.mccormick at kitware.com; sean at rogue-research.com
Cc: insight-developers at itk.org
Subject: Re: [Insight-developers] type of modification time

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?

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
>
>
_______________________________________________
Powered by www.kitware.com

Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html

Kitware offers ITK Training Courses, for more information visit:
http://kitware.com/products/protraining.php

Please keep messages on-topic and check the ITK FAQ at:
http://www.itk.org/Wiki/ITK_FAQ

Follow this link to subscribe/unsubscribe:
http://www.itk.org/mailman/listinfo/insight-developers