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

feat: Define all surface methods out-of-line #676

Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Jan 27, 2021

As previously discussed, most Surface related methods are currently defined as inline in .ipp files.

This PR adds a cmake option ACTS_SURFACE_INLINE. If enabled, the .hpp files will not include their corresponding .ipp files, but their corresponding .cpp files will. Using a special preprocessor directive, the inline keywords are removed from the methods, turning them into regular symbols.

This PR moves all method definitions into the corresponding .cpp files.

In local testing, I don't observe runtime performance degradation and slight compile-time improvement. Let's see what the CI has to say on the latter topic.

The use of preprocessor macros might be controversial, so let's discuss. The reason I think this approach might be a good idea is that this enables us to possible to back to the inlined methods. If we choose to go this route, we should probably ensure that both variants work in the CI.

Fixes #409

@paulgessinger paulgessinger added this to the next milestone Jan 27, 2021
@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Jan 27, 2021
@paulgessinger paulgessinger changed the title feat: Make inlining of all surface method optional feat: Make inlining of all surface methods optional Jan 27, 2021
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #676 (69b17c5) into master (1ec8987) will decrease coverage by 0.01%.
The diff coverage is 29.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
- Coverage   49.03%   49.02%   -0.02%     
==========================================
  Files         331      325       -6     
  Lines       16549    16547       -2     
  Branches     7725     7727       +2     
==========================================
- Hits         8115     8112       -3     
+ Misses       3013     3012       -1     
- Partials     5421     5423       +2     
Impacted Files Coverage Δ
Core/include/Acts/Surfaces/ConeSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/CylinderSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/DiscSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/LineSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/PlaneSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/Surface.hpp 60.00% <ø> (ø)
Core/src/Surfaces/ConeSurface.cpp 34.28% <17.24%> (-12.06%) ⬇️
Core/src/Surfaces/DiscSurface.cpp 30.61% <24.29%> (-7.60%) ⬇️
Core/src/Surfaces/CylinderSurface.cpp 34.57% <27.36%> (-7.37%) ⬇️
Core/src/Surfaces/LineSurface.cpp 35.84% <31.65%> (-29.16%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ec8987...ebbc2ae. Read the comment docs.

@paulgessinger
Copy link
Member Author

CI reports memory usage to be the same, but hints toward slightly lower time to compile. Not super conclusive though.

@paulgessinger
Copy link
Member Author

@msmk0

@msmk0 msmk0 self-requested a review February 1, 2021 08:59
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

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

I see no technical problems with this.

Personally, I would strongly prefer to not make this configurable and to move the inline code to regular .cpp files instead. As suggested by the CI build and by @paulgessinger observations, moving the implementations to .cpp files has no measurable performance impact in realistic setups. The proposed setup adds complexity that we need to maintain in the future. If there is a hotspot in one of the methods that can be improved by inlining, we can move that particular function as an inline function into the .hpp file in the future.

@paulgessinger paulgessinger changed the title feat: Make inlining of all surface methods optional feat: Define all surface methods out-of-line Feb 9, 2021
@paulgessinger
Copy link
Member Author

@msmk0: I removed the #define, the definitions are now in the .cpp files.

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Feb 9, 2021
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Moving code around, looks good.

@asalzburger
Copy link
Contributor

Wanted to wait to see 'if' we see some change in build_performance, but we can merge otherwise.

@asalzburger asalzburger merged commit b1fc4e7 into acts-project:master Feb 9, 2021
@paulgessinger paulgessinger deleted the surface-inline-methods branch February 9, 2021 16:07
@paulgessinger paulgessinger modified the milestones: next, v6.0.0 Mar 2, 2021
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.

Move Surface inline code to implementation files
3 participants