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.

Sign up for GitHub

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

Jump to bottom

PERF: Make ImageRegion trivially copyable, remove inheritance (ITK_FUTURE_LEGACY_REMOVE) #4344

Merged
merged 1 commit into from
Nov 30, 2023
Merged

PERF: Make ImageRegion trivially copyable, remove inheritance (ITK_FUTURE_LEGACY_REMOVE) #4344

merged 1 commit into from
Nov 30, 2023

Conversation

N-Dekker
Copy link
Contributor

Made all member functions of ImageRegion non-virtual, and removed its inheritance from Region.

The size of ImageRegion<2> was observed to go down from 40 bytes (before this commit) to 32 bytes (after this commit), on Visual C++ 2022 (x64). A run-time performance improvement of more than 25% on default-constructing and destructing a sequence of ImageRegion objects was observed, by benchmarking std::make_unique<itk::ImageRegion<2>[]>(n).

@github-actions github-actions bot added type:Performance Improvement in terms of compilation or execution time 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 Nov 22, 2023
@N-Dekker N-Dekker changed the title PERF: Make ImageRegion lightweight and trivially copyable WIP: PERF: Make ImageRegion lightweight and trivially copyable Nov 22, 2023
dzenanz
dzenanz reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Let's see how testing goes.

itk::Region itself is not much used. Did you consider making it trivially copyable? That way we could keep the inheritance.

@N-Dekker
Copy link
Contributor Author

itk::Region itself is not much used. Did you consider making it trivially copyable? That way we could keep the inheritance.

Technically that might work as well, but it's a bit risky to keep the inheritance, when we make itk::Region trivially copyable. You know, old books taught us to declare destructors of base classes virtual, to avoid undefined behavior when deleting a derived object through a pointer to its base. We could mitigate such a risk by using private inheritance, instead of public inheritance. Or by declaring the base class destructor protected. But I guess that would beat the original purpose of the inheritance in the first place. 🤷

My feeling is that the itk::Region base class might have been a nice idea originally, it might have some rare use cases, but ImageRegion is being used so extensively, also in performance critical code, that it's worth considering to drop the inheritance feature.

@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from 33ddd3d to 7fead70 Compare November 22, 2023 20:46
@github-actions github-actions bot removed the type:Performance Improvement in terms of compilation or execution time label Nov 22, 2023
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Nov 22, 2023 •
edited
Loading

Interestingly, only when ImageRegion is made trivially copyable, compiler warnings about unused ImageRegion variables appear. At https://open.cdash.org/viewBuildError.php?type=1&buildid=9172873 (ITK.macOS/Darwin/Xcode_13.2.1/clang-1300.0.29.30):

itkZeroFluxNeumannBoundaryCondition.hxx:97:14: warning: unused variable 'requestRegion' [-Wunused-variable]

A fix is included with this pull request.

Update, November 24, 2023: Those warnings are now addressed by a separate pull request:

@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from 7fead70 to 67c415a Compare November 23, 2023 10:01
@github-actions github-actions bot added the area:Filtering Issues affecting the Filtering module label Nov 23, 2023
@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from 67c415a to d73f19a Compare November 23, 2023 16:34
@github-actions github-actions bot added area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Nov 23, 2023
@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from d73f19a to 05a3426 Compare November 23, 2023 23:58
@N-Dekker N-Dekker mentioned this pull request Nov 24, 2023
Merged
@blowekamp
Copy link
Member

I would consider this a breaking API change, that is only suitable for a major revision.

@N-Dekker
Copy link
Contributor Author

I would consider this a breaking API change, that is only suitable for a major revision.

Thanks Bradley, good point. This is indeed an API change. Which specific ITK revision could then include a change like this?

@dzenanz
Copy link
Member

dzenanz commented Nov 24, 2023

Perhaps make it future legacy? So it becomes legacy in 6.0 (soon), and gets removed in 7.0.

@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from 05a3426 to b4112d0 Compare November 24, 2023 20:35
@N-Dekker N-Dekker marked this pull request as ready for review November 24, 2023 20:37
@N-Dekker N-Dekker changed the title WIP: PERF: Make ImageRegion lightweight and trivially copyable PERF: Make ImageRegion trivially copyable, remove inheritance (ITK_FUTURE_LEGACY_REMOVE) Nov 24, 2023
@N-Dekker N-Dekker marked this pull request as draft November 26, 2023 14:04
@N-Dekker
Copy link
Contributor Author

Reverted status to "work in progress", because I would prefer to first see the related ("spin-off") pull request being processed:

(Afterwards I would rebase this one.)

@N-Dekker
PERF: Make ImageRegion trivially copyable, remove inheritance (FUTURE)
1f3bbb6 
Made `ImageRegion` more lightweight, by declaring all of its member functions
non-virtual, and by removing its inheritance from `Region`, when
`ITK_FUTURE_LEGACY_REMOVE` is enabled.

The size of `ImageRegion<2>` was observed to go down from 40 bytes (before this
commit) to 32 bytes (after this commit), on Visual C++ 2022 (x64). A run-time
performance improvement of more than 25% on default-constructing and destructing
a sequence of `ImageRegion` objects was observed, by benchmarking
`std::make_unique<itk::ImageRegion<2>[]>(n)`.
@N-Dekker N-Dekker force-pushed the Make-ImageRegion-trivially_copyable branch from b4112d0 to 1f3bbb6 Compare November 28, 2023 14:12
@github-actions github-actions bot removed area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module labels Nov 28, 2023
@github-actions github-actions bot added type:Performance Improvement in terms of compilation or execution time and removed area:Segmentation Issues affecting the Segmentation module area:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Nov 28, 2023
@N-Dekker N-Dekker marked this pull request as ready for review November 28, 2023 20:20
thewtex
thewtex approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

🚀 🔥

@N-Dekker very nice!

Note: the previously failing first-interaction check is now fixed via actions/first-interaction#287

@thewtex thewtex merged commit 5ccd18c into InsightSoftwareConsortium:master Nov 30, 2023
11 checks passed
@N-Dekker N-Dekker mentioned this pull request Jan 2, 2024
Closed
@N-Dekker N-Dekker mentioned this pull request Apr 30, 2024
Merged
2 tasks
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request May 22, 2024
@N-Dekker
PERF: Make ImageRegion trivially copyable (LEGACY_REMOVE)
0af436f 
Replaced `ITK_FUTURE_LEGACY_REMOVE` with `ITK_LEGACY_REMOVE`, regarding its
inheritance from `itk::Region`.

A run-time performance improvement of more than 25% on default-constructing
and destructing a sequence of `ImageRegion` objects was observed.

- Follow-up to pull request InsightSoftwareConsortium#4344
commit 1f3bbb6
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request May 22, 2024
@N-Dekker
PERF: Make ImageRegion trivially copyable (LEGACY_REMOVE)
9c73fce 
Replaced `ITK_FUTURE_LEGACY_REMOVE` with `ITK_LEGACY_REMOVE`, regarding its
inheritance from `itk::Region`.

A run-time performance improvement of more than 25% on default-constructing
and destructing a sequence of `ImageRegion` objects was observed.

- Follow-up to pull request InsightSoftwareConsortium#4344
commit 1f3bbb6
@N-Dekker N-Dekker mentioned this pull request May 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

@thewtex thewtex thewtex approved these changes

Assignees
No one assigned
Labels
area:Core Issues affecting the Core module type:Performance Improvement in terms of compilation or execution time 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@N-Dekker @blowekamp @dzenanz @thewtex