Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PERF: Use initializer list in ShapedImageNeighborhoodRange operator* #144

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Nov 7, 2018

Results in ~10% performance increase.

@N-Dekker this came up when testing the new constant boundary condition in ITKUltrasound.

@@ -347,7 +347,7 @@ class ShapedImageNeighborhoodRange final
/** Returns a reference to the current pixel. */
reference operator*() const ITK_NOEXCEPT
{
return reference(m_ImageBufferPointer, m_ImageSize, m_OffsetTable, m_NeighborhoodAccessor, m_Location + *m_CurrentOffset);
return reference{m_ImageBufferPointer, m_ImageSize, m_OffsetTable, m_NeighborhoodAccessor, m_Location + *m_CurrentOffset};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with using curly braces instead of parentheses, but it only looks like a STYLE issue to me. (Actually I originally wanted to use curly braces on that line of code but KWStyle complained, at that time. If I remember correctly!) So are you sure that this change really has such an impact on the performance? If so, do you have a clue why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, older versions of KWStyle will complain about }; 😞 , but this has been fixed in newer versions ☀️ .

I did observe the speed up, but I do not know the details of why this makes a difference to the compiler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to hear KWStyle no longer complains about this }; syntax!

With which specific compiler version did you observe the speed-up? Because it sounds like a compiler issue to me! I just tried, but did not observe a significant performance difference between curly braces and parentheses on VS2017 (Version 15.8.9).

Anyway, the code change you're proposing here is perfectly fine to me.

@hjmjohnson hjmjohnson merged commit 3991369 into InsightSoftwareConsortium:master Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants