[Insight-developers] Image memory model
Karthik Krishnan
karthik.krishnan at kitware.com
Mon Jun 2 11:53:25 EDT 2008
On Sun, Jun 1, 2008 at 7:28 PM, Jim Miller <millerjv at gmail.com> wrote:
> Karthik,
>
> Have you verified that running the NeighborhoodIterators through the
> Functors/Accessors did not affect performance? If the extra level of
> indirection is not properly optimized out for the default case, then we
> could have introduced a performance overhead. Did you running any timing
> tests before/after this change?
Yes I did. I do not remember noticing any significant reduction in
speed after this change (in release optimized mode).
All methods are inlined and when unrolled yield the same instructions
as before. So we shouldn't be expecting any change either with
function inlining.
As an example, for the ConstNeighborhoodIterator; What used to be:
PixelType GetCenterPixel() const
{ return *( this->GetCenterPointer() ); }
PixelType GetPixel( int n, ...) const
{
....
return (m_NeighborhoodAccessorFunctor.Get(this->operator[](n)));
}
was changed to
PixelType GetCenterPixel() const
{ return m_NeighborhoodAccessorFunctor.Get( this->GetCenterPointer() ); }
PixelType GetPixel(...) const
{
....
return (*(this->operator[](n)));
}
with the NeighborhoodAccessorFunctor reading:
inline PixelType NeightborhoodAcecssorFunctor::Get( const
InternalPixelType *pixelPointer ) const
{
return (*pixelPointer);
}
Same holds true all the several other Set/Get access methods and
Boundary Condition accesses in all the iterators.
Basically any direct dereferencing is forced to go through the
accessors. The default implementation of the accessor just
dereferences and returns the value. The converse is done for the Set
methods.
These tests have been carried only on gcc. I made sure the functions
got inlined compiling with -WInline.
Thanks
--
karthik
>
> Jim
>
> On Sun, Jun 1, 2008 at 1:19 PM, Karthik Krishnan
> <karthik.krishnan at kitware.com> wrote:
>>
>> On Sun, Jun 1, 2008 at 12:35 PM, Jim Miller <millerjv at gmail.com> wrote:
>> > I think the issue with this design is that the NeighborhoodIterators do
>> > not
>> > go through the PixelAccessors (for efficiency). In fact, the
>> > NeighborhoodIterators keep a "neighborhood" or pointers to pixels and
>> > know
>> > how to update those pointers via the iteration.
>>
>> I had added these a while ago, to support the VectorImage.
>> Please see itk::NeighborhoodIteratorFunctor and
>> itkVectorImageNeighborhoodAccessorFunctor.
>>
>> All iterators in ITK go through accessors/accessor functors.
>>
>> The NeighborhoodAccessorFunctors work just like the PixelAccessors,
>> however have a slightly different API, so as to gel with the
>> neighborhoodAccessors.
>>
>> inline PixelType Get( const InternalPixelType * ) const
>>
>> inline void Set( InternalPixelType* &pixelPointer, const PixelType
>> &p ) const
>>
>> inline PixelType BoundaryCondition(
>> const OffsetType& point_index,
>> const OffsetType &boundary_offset,
>> const NeighborhoodType *data,
>> const ImageBoundaryConditionConstPointerType boundaryCondition) const
>>
>> Dan, you must be familiar with the Set/Get methods, having written the
>> PixelAccessor for the SliceContiguousImage. The last method,
>> BoundaryCondition method is called whenever the operator() method is
>> accessed for the boundary conditions, so you can return the right
>> pointer there. In the accessors, we delegate this back to the boundary
>> condition (the slower alternative signature is used here). You will
>> notice that there are two signatures for each of the boundary
>> conditions, One of them takes the accessor itself as an argument.
>>
>> These accessors are obtained as traits of the image. All iterators are
>> tested for itk::VectorImage and itk::Image.
>>
>> So, I think this should be possible for the SliceContiguousImage as
>> well, by writing a itk::SliceContiguousNeighborhoodAccessorFunctor.
>>
>> Thanks
>> --
>> karthik
>>
>> > Introducing the GetBufferPointer() method is one of things I regret most
>> > in
>> > the design of the ITK image. I never should have done that. If all the
>> > iterators worked of the concept of an "offset" from the first pixel
>> > (i.e.
>> > the number of pixels since the beginning of the buffer), then they could
>> > have indexed the pixels as PixelContainer[i] to get the ith pixel. The
>> > PixelContainer could then hide the memory layout from the Image class
>> > and
>> > from the Iterators. This would even allow PixelContainers that
>> > internally
>> > compressed the image data or store part of the image data on disk and
>> > part
>> > in memory.
>> >
>> > ImageRegionIterator can support such a referencing scheme easily.
>> > ImageRegionIteratorWithIndex would require a little bit of work to
>> > support
>> > the memory model. NeighborIterator would require a good deal of work to
>> > support such a model. These are all doable. It is just a matter of
>> > resources and a matter of efficiency (eg. do the neighborhood iterators
>> > get
>> > too slow in this new model).
>> >
>> > Outside of the Iterators, the IO mechanisms (including the Import/Export
>> > facilities for interfacing to other systems), and few poorly written
>> > filters, the use of GetBufferPointer() should be fairly limited and
>> > isolated.
>> >
>> > In practice, when I interface into systems that have a slice contiguous
>> > model, I either use ITK just on 2D slices and forgo 3D processing, or I
>> > make
>> > a volume contiguous image that is a copy of the slice contiguous image.
>> > These are not the best solutions but todays 64 bit world is currently
>> > much
>> > larger than medical image data set sizes.
>> >
>> >
>> >
>> > On Sun, May 25, 2008 at 8:28 PM, Karthik Krishnan
>> > <karthik.krishnan at kitware.com> wrote:
>> >>
>> >> Dan:
>> >>
>> >> On Sun, May 25, 2008 at 2:41 AM, Dan Mueller <dan.muel at gmail.com>
>> >> wrote:
>> >> >
>> >> > Image::GetBufferPointer() is used extensively within the toolkit.
>> >> > Take
>> >> > a look at the constructors for ImageConstIterator,
>> >> > ConstNeighborhoodIterator, ImageReverseConstIterator,
>> >> > ImageConstIteratorWithIndex, etc. I have added a list to
>> >> > http://www.itk.org/Wiki/Proposals:Slice_contiguous_images
>> >> > however I realise now that some of these are
>> >> > ImportImageContainer::GetBufferPointer() (I'll fix this up when time
>> >> > permits).
>> >>
>> >> That should not be a cause of concern
>> >> image->GetBufferPointer() is just a pointer to the first pixel in
>> >> the image. It is the dereferencing (m_BufferPointer + offset) that's
>> >> of concern.
>> >>
>> >> >
>> >> > Unfortunately, while your suggestion of using the PixelAccessor and
>> >> > PixelAccessorFunctor would have been easy, I think it is too good to
>> >> > be true.
>> >>
>> >> It is true.
>> >>
>> >> > The assumptions regarding contiguous memory allocation are
>> >> > deeply entrenched.
>> >>
>> >> These classes were introduced a year or two ago, precisely because we
>> >> had to support an alternate memory model image for NAMIC.
>> >>
>> >> >
>> >> > Take for example the very first step you discussed, the act of
>> >> > passing
>> >> > the input pixel value from the iterator.Get() function to the
>> >> > PixelAccessorFunctor:
>> >> >> PixelType iterator.Get() reads
>> >> >> { return pixelAccessorFunctor.Get(*(m_Buffer+m_Offset)); }
>> >>
>> >> The variable is passed by reference, however it is later dereferenced
>> >> by the functor to the corrent pointer. The VectorPixelAccessor reads:
>> >>
>> >> inline ExternalType Get( const InternalType & input, const unsigned
>> >> long offset ) const
>> >> {
>> >> ExternalType output( (&input)+(offset*m_OffsetMultiplier) ,
>> >> m_VectorLength );
>> >> return output;
>> >> }
>> >>
>> >> ie the offset is passed in and based on the vector length instead of
>> >> returning *(m_Buffer+m_Offset) that an itk::Image would do, the
>> >> itk::VectorImage returns *(m_Buffer + m_Offset * m_VectorLength ) .
>> >>
>> >> >
>> >> > This deference *(m_Buffer+m_Offset) will access potentially invalid
>> >> > memory in the slice-contiguous case (m_Buffer is the result of
>> >> > m_Image->GetBufferPointer(), the start of the contiguous array, or
>> >> > the
>> >> > start of the first slice for my case).
>> >>
>> >> I realize that it would have been better to pass in the offset and the
>> >> buffer_start_pointer in, to avoid invalid de-references. (I think
>> >> passing them by reference avoids the de-reference). So we could change
>> >> the signature of the the accesssor to
>> >>
>> >> inline ExternalType Get( const InternalType & begin, const unsigned
>> >> long offset ) const
>> >>
>> >> and the iterator would read:
>> >>
>> >> PixelType Get(void) const
>> >> { return m_PixelAccessorFunctor.Get(m_Buffer, m_Offset); }
>> >>
>> >> instead of
>> >>
>> >> PixelType Get(void) const
>> >> { return m_PixelAccessorFunctor.Get(*(m_Buffer+m_Offset)); }
>> >>
>> >>
>> >> > There are other places this assumption is also made: for example
>> >> > ImageConstIteratorWithIndex::SetIndex() uses the buffer pointer to
>> >> > compute the index.
>> >>
>> >> Again the starting pointer should not matter. All attempts to
>> >> dereference the pointers, via Set or Get go through the accessors.
>> >>
>> >> >
>> >> > Have I misinterpreted your meaning? I think either m_Buffer and/or
>> >> > m_Offset would need to altered from their original meaning to work
>> >> > for
>> >> > slice-contiguous memory...
>> >>
>> >> No. I don't think so. In your case,
>> >>
>> >> m_Buffer would be any arbitrary value say 0.
>> >> m_Offset would be the Nth pixel into the image.
>> >>
>> >> Any attempt to access the Nth pixel would go through the accessor,
>> >> which would have start pointers to all the slices in your image and
>> >> would read something like:
>> >>
>> >> inline ExternalType itk::SlicePixelAccessor::Get( const InternalType
>> >> & begin, const unsigned long offset ) const
>> >> {
>> >> const unsigned long slice = offset/(x_size*y_size);
>> >> const unsigned long offset_into_slice = offset % (x_size*y_size);
>> >> return *(m_SlicePointer[slice] + offset_into_slice);
>> >> }
>> >>
>> >>
>> >> --
>> >> Hope this helps.
>> >>
>> >> Thanks
>> >> --
>> >> Karthik Krishnan
>> >> R&D Engineer,
>> >> Kitware Inc.
>> >> _______________________________________________
>> >> Insight-developers mailing list
>> >> Insight-developers at itk.org
>> >> http://www.itk.org/mailman/listinfo/insight-developers
More information about the Insight-developers
mailing list