[Insight-developers] Small modification to ImportImageContainer

[Tom Vercauteren]

Luis,

Rechecking the code, it seems that the current deallocate function
really needs more work...

At first I wanted to mimic AllocateElements:
  - AllocateElements does not directly allocate m_ImportPointer but
rather returns a new pointer.
  - This new pointer gets in turn assigned to m_ImportPointer by the
calling function.
  - Since AllocateElements does not depend on any class member, it can
be made const.

The issues with the current implementation is that:
  - It internally checks whether m_ContainerManageMemory is true.
  - m_ContainerManageMemory is used to give information about the
argument of DeallocateElements

I see at least two ways of making it cleaner. Both involve changing
"virtual void DeallocateElements (const TElement* ptr) const;"

1) - Use "virtual void DeallocateElements(TElement* ptr,
TElementIdentifier ptrcapacity) const;"
    - Move the "if (m_MemoryAllocatedByAllocator)" out of the
DeallocateElements functions.
    - Note that the const is removed from the arguments to please vs6
and ease the job for an std::allocator.


2)  - Use "virtual void DeallocateManagedMemory();"
     - The  "if (m_MemoryAllocatedByAllocator)" would be kept within
this function
     - This function could also potentially set m_Size  and m_Capacity


I have put an example inheriting class using the second option below.
It needs some polishing but should make things clearer.

Tom

template<class TElementIdentifier, typename TElement>
class TestImportImageContainer : public itk::ImportImageContainer<
TElementIdentifier, TElement >
{
public:
  /** Standard class typedefs.   */
  typedef TestImportImageContainer                             Self;
  typedef itk::ImportImageContainer< TElementIdentifier, TElement > Superclass;
  typedef itk::SmartPointer<Self>               Pointer;
  typedef itk::SmartPointer<const Self>         ConstPointer;

  /** Method for creation through the object factory. */
  itkNewMacro(TestImportImageContainer);

  /** Run-time type information (and related methods). */
  itkTypeMacro(TestImportImageContainer, ImportImageContainer);

  typedef typename Superclass::ElementIdentifier  ElementIdentifier;
  typedef typename Superclass::Element            Element;

  typedef std::allocator<TElement>                Allocator;

  // Methods from itkObject
  virtual ~TestImportImageContainer()
    {
    itkTotalMemoryUsed -= m_TotalSize;
    m_TotalSize = 0;
    DeallocateManagedMemory();
    }
  TestImportImageContainer()
     : m_TotalSize(0)
     ,m_MemoryAllocatedByAllocator(false)
    {
    }
protected:
  TElement* AllocateElements(ElementIdentifier size) const
    {
    std::cout << "ImportImageContainer: Allocating "
              << size << " elements of type "
              << typeid(TElement).name() << " totaling "
              << sizeof(TElement) * size << " bytes" << std::endl;

    TElement* data;
    try
      {
      data = m_Allocator.allocate(size);
      if (data)
        {
        new (data) Element[size];
        }
      }
    catch(...)
      {
      data = 0;
      }
    if(!data)
      {
      // We cannot construct an error string here because we may be out
      // of memory.  Do not use the exception macro.
      throw itk::MemoryAllocationError(__FILE__, __LINE__,
                                       "Failed to allocate memory for image.",
                                       ITK_LOCATION);
      }

    m_TotalSize = size * sizeof(TElement);
    itkTotalMemoryUsed += m_TotalSize;

    m_MemoryAllocatedByAllocator = true;

    std::cout << "ImportImageContainer: Total memory used is "
              << itkTotalMemoryUsed << " bytes" << std::endl;

    return data;
    }

  void DeallocateManagedMemory()
    {
    std::cout << "ImportImageContainer: Deallocating "
              << this->Capacity() << " elements of type "
              << typeid(TElement).name() << " totaling "
              << sizeof(TElement) * this->Capacity() << " bytes" << std::endl;

    if (m_MemoryAllocatedByAllocator)
      {
      TElement * ptr = this->GetImportPointer();
      const TElement * const end = ptr + this->Capacity();
      for (TElement * base = ptr; base<end; ++base)
        {
        m_Allocator.destroy(base);
        }
      m_Allocator.deallocate(ptr, this->Capacity() );
      m_MemoryAllocatedByAllocator = false;
      }
    else
      {
      Superclass::DeallocateManagedMemory();
      }
    }

private:
  TestImportImageContainer(const TestImportImageContainer&);
  void operator=(const TestImportImageContainer&);
  mutable TElementIdentifier m_TotalSize;

  mutable Allocator          m_Allocator;
  mutable bool               m_MemoryAllocatedByAllocator;
};



On Tue, Mar 31, 2009 at 15:18, Tom Vercauteren <tom.vercauteren at m4x.org> wrote:
> Hi Luis,
>
>> After looking closer at this two methods I see now why they can
>> both be const:  They do not affect member variables (at least in
>> an obvious direct manner). This is still a bit deceptive, because
>> when DeallocateElements() is called, we pass a member variable as
>> its argument..., something that is also strange.... why is that
>> we simply not have the method without arguments, and internally
>> we access in it the member variable ?
>
> We could do that, I just thought that passing the pointer as an
> argument was more generic.
>
>
>> It seems however that the argument of DeallocateElements can
>> be made non-const, and in that way you won't need to cast const
>> away.
>
> I chose to make it const since it was standard compliant and seemed
> more appropriate. The fact that we need the const-cast for vs6 makes
> it however much less elegant...
>
>
>> Do you need this pointer argument to be const in the context
>> in which you will use this method in the derived class that you
>> are developing ?
>
> I should try without it, but I don't think it will really matter for
> my application.
>
>
>> I guess that we may understand better if you post the derived
>> class that you are implementing.
>
> It's a bit complex to post my Image subclass here as it uses some
> internal non-ITK code.
> It basically uses std::allocator<TElement> instead of calling new and delete.
>
> If you find it useful, I could try to modify TestImportImageContainer
> in itkFactoryTestLib so as to use such a custom allocator:
> http://www.itk.org/cgi-bin/viewcvs.cgi/Testing/Code/Common/itkFactoryTestLib.cxx?root=Insight&sortby=date&view=markup
>
>
>> The ImportImageContainer class is at the very heart of ITK so we
>> should be very careful with any changes we made to it.   :-/
>
> I most certainly understand that care should be taken for this class!
> This is why I asked for feedback.
>
> Tom
>