`NumericTraits<VariableLengthVector<T>>::SetLength` shall avoid allocations

LucHermitte commented 3 years ago

Description

Instead of m.SetSize(s);, NumericTraits<VariableLengthVector<T>>::SetLength() should use either

m.SetSize(s, false);
or the more explicit SetSize(s, ShrinkToFit(), KeepOldValues());
or may be other policies, I'm not sure what a better default would be.

Impact analysis

Any generic filter that uses any scalar pixel, or even VariableLengthVector pixels and resize them in a loop, could avoid unwelcomed deallocations+reallocations. Of course, the filter could be written differently.

Expected behavior

Don't deallocate + reallocate the exact same amount of memory for an existing VLV pixel.

The exact performance improvement will highly depend on the filters using the resizing function from the trait.

Actual behavior

When I've optimized VLV internals a few years ago, and provided reallocation policies at the time, I did not want to change default behaviours in places I wasn't sure. In particular I've decided that the default behaviour of vlv.SetSize(42); would be the same as before: deallocate and reallocate. I'm still not sure for this case as the end user can always use the other flavours instead.

However, when using the generic NumericTraits<VariableLengthVector<T>>::SetLength(), there is no way to specify the reallocation policy, and I think now this is not a god default behaviour.

Versions

From 4.? till the current version I see on master (5.2rc02)

LucHermitte commented 3 years ago

Also, BTW: itk::DefaultConvertPixelTraits<VariableLengthVector<>>::GetNumberOfCOmponents() should not take the pixel by value!

Copying VLVs is expensive.

thewtex commented 3 years ago

I also receive the following warning with GCC 9.3:

[222/272] Linking CXX shared module Wrapping/Generators/Python/itk/_ITKImageGradientPython.so
/home/matt/src/ITK/Modules/Filtering/ImageGradient/include/itkGradientRecursiveGaussianImageFilter.hxx: In member function ‘GenerateData’:
/home/matt/src/ITK/Modules/Core/Common/include/itkDefaultPixelAccessor.h:72:5: warning: ‘MEM[(const struct CovariantVector &)&correctedGradient + 12]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   72 |     output = input;
      |     ^
/home/matt/src/ITK/Modules/Filtering/ImageGradient/include/itkGradientRecursiveGaussianImageFilter.h:214:29: note: ‘MEM[(const struct CovariantVector &)&correctedGradient + 12]’ was declared here
  214 |     OutputPixelType         correctedGradient;
      |                             ^

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

InsightSoftwareConsortium / ITK