Skip to content
Snippets Groups Projects

Return track parameter position and momentum vectors by value

Merged Return track parameter position and momentum vectors by value
sswatman/athena:refactor/trkpar_posmom_rbv into master
All threads resolved!
Merged Stephen Nicholas Swatman requested to merge sswatman/athena:refactor/trkpar_posmom_rbv into master
All threads resolved!

In the current implementation of the track parameters, the position and momentum vectors are returned by reference. This is enforced at an interface level by the ParametersBase class. Unfortunately this is not necessarily good interface design, for several reasons:

Firstly, this interface design forces certain design choices on the implementing classes. That is to say, the implementing classes need to store some vector internally in order for it to be possible for them to return them by reference. This robs the implementing classes of the freedom to design their internals in the most efficient possible way.

Secondly, it breaks encapsulation of the class. The internals of the track parameters class should be hidden from the outside world, and this concept of information hiding is integral to good object-oriented programming. However, returning references to members of the class breaks this rule. Returning the references as const alleviates this problem somewhat, but the implementation is still far from ideal.

Thirdly, there is a potential lifetime issue with this design, as a reference to the internal vectors of the track parameters object can outlive the parameters object which they point to, which can lead to memory unsafety. Unless a Rust-like reference lifetime is implemented on a code level (because it is not supported by the C++ language standard) there is virtually no way to go about this safely.

Finally, there are thread-safety concerns with this design, as it is possible for one thread to update the internals of the object while another thread has a seemingly const reference to its internal data. This is not supposed to happen in the Athena code, but it's hard to enforce such requirements.

The solution proposed in this commit is to move away from the return-by-reference design, towards a return-by-value design. Returning a copy instead of a reference to internal data resolves all the previously mentioned concerns with the existing design, and should not incur any significant performance hits.

At first it might seem like replacing this interface would be a Herculean effort because these methods are used in so many different places, but there is a saving grace that means that this change might be simpler than expected. Indeed, the existing implementation uses const references, which means that we can exploit the fact that temporaries (and other rvalues) can be implicitly bound to const references to extend their lifetime. That is to say, any existing code that relies on this code returning const references should be compatible on a source code level (not on an API level) with the new code returning these vectors by value.

It's pretty much impossible to properly test changes to the track parameters locally since there are so many usage sites throughout the code. So I'm relying on the CI system to give me some indication on whether this works or not. It works for me compiling a subset of the Athena code, but compiling the whole thing is difficult.

Tagging @sroe, @christos, @goblirsc for visibility.

Merge request reports

Pipeline #2083838 passed

Stage: external

    Pipeline passed for e178aba1 on sswatman:refactor/trkpar_posmom_rbv

    Approval is optional

    Merged by Vakhtang TsulaiaVakhtang Tsulaia 4 years ago (Nov 12, 2020 6:29pm UTC)

    Merge details

    • Changes merged into master with 167c61c0.
    • Deleted the source branch.

    Activity

    Filter activity
    • Approvals
    • Assignees & reviewers
    • Comments (from bots)
    • Comments (from users)
    • Commits & branches
    • Edits
    • Labels
    • Lock status
    • Mentions
    • Merge request status
    • Tracking
    Please register or sign in to reply