Skip to content

PERF: FUTURE: Default default-constructor of SymmetricSecondRankTensor#4473

Merged
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Default-SymmetricSecondRankTensor-default-constructor
Feb 26, 2024
Merged

PERF: FUTURE: Default default-constructor of SymmetricSecondRankTensor#4473
dzenanz merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Default-SymmetricSecondRankTensor-default-constructor

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

Follow-up to pull request #4469 commit 755cd10
"PERF: FUTURE: Default default-constructors of RGBPixel and RGBAPixel"

@github-actions github-actions Bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Feb 21, 2024
Comment thread Modules/Core/Common/include/itkSymmetricSecondRankTensor.h Outdated
@blowekamp
Copy link
Copy Markdown
Member

I assume this PR is similar to the RGBPixel improvement with the goal of improving the performance of the Image::Allocate method. Perhaps this is a case there the optimization should go into the Allocate method. In SimpleITK and when interfacing with other libraries and buffers, the Image container buffer is considered a raw buffer. Perhaps if certain conditions, c++ traits or itk PixelTraits then a special optimization in this method could occur where the buffer is initialized as some type of raw buffer, then case to the component type.

These initialized objects can be an insidious thing to track down as the behavior is not deterministic and varies with debug and release builds. So I have some concern with the proposed future changes.

@N-Dekker N-Dekker force-pushed the Default-SymmetricSecondRankTensor-default-constructor branch from 3e8ad23 to 0b2e0f2 Compare February 21, 2024 19:53
Comment thread Modules/Core/Common/test/itkCommonTypeTraitsGTest.cxx
@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 21, 2024

I assume this PR is similar to the RGBPixel improvement with the goal of improving the performance of the Image::Allocate method.

Indeed, I have indeed seen huge ("future") performance improvements of Image::Allocate from the similar RGBPixel PR (#4469), and I would expect similar ("future") performance improvements for SymmetricSecondRankTensor, with this commit. But it's not just that. It's also a matter of consistency between pixel types:

    TPixel pixel1; // May not be initialized. 
    TPixel pixel2{}; // Properly zero-initialized.
    TPixel pixel3 = TPixel{}; // Properly initialized.
    TPixel pixel4 = TPixel(); // Properly initialized.

As I mentioned at #4469 (comment)

Note that std::make_unique<TPixel>() and std::make_shared<TPixel>() will zero-initialize the created pixel(s) anyway, also with this PR.

Slightly off-topic, I think it would help if itk::Image would have an extra member function, AllocateZeroInitialized(), which would be equivalent to Allocate(bool initializePixels) with true as argument. It seems cleared to me to explicitly write image.AllocateZeroInitialized(), instead of image.Allocate(true). What do you think?

Follow-up to pull request InsightSoftwareConsortium#4469
commit 755cd10
"PERF: FUTURE: Default default-constructors of `RGBPixel` and `RGBAPixel`"
@N-Dekker N-Dekker force-pushed the Default-SymmetricSecondRankTensor-default-constructor branch from 0b2e0f2 to e701c15 Compare February 21, 2024 20:41
@N-Dekker N-Dekker changed the title WIP: FUTURE: Default default-constructor of SymmetricSecondRankTensor PERF: FUTURE: Default default-constructor of SymmetricSecondRankTensor Feb 21, 2024
@N-Dekker N-Dekker marked this pull request as ready for review February 21, 2024 20:43
@dzenanz dzenanz merged commit 0a30e7f into InsightSoftwareConsortium:master Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants