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

refactor: Use pointer for (C)KF inputMeasurements #742

Merged
merged 2 commits into from
Mar 9, 2021

Conversation

XiaocongAi
Copy link
Contributor

This PR changes the inputMeasurements in (C)KF actor to use pointer. This could save the data copy time vastly.

@XiaocongAi XiaocongAi added Component - Core Affects the Core module Improvement Changes to an existing feature labels Mar 9, 2021
@XiaocongAi XiaocongAi added this to the next milestone Mar 9, 2021
@XiaocongAi
Copy link
Contributor Author

XiaocongAi commented Mar 9, 2021

A comparison of the timing test for (C)KF using smeared truth parameters for ttbar events W/O the change below:
image
image

@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #742 (734c632) into master (a9ec279) will not change coverage.
The diff coverage is 72.72%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #742   +/-   ##
=======================================
  Coverage   48.86%   48.86%           
=======================================
  Files         325      325           
  Lines       16682    16682           
  Branches     7791     7791           
=======================================
  Hits         8151     8151           
  Misses       3061     3061           
  Partials     5470     5470           
Impacted Files Coverage Δ
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 29.21% <66.66%> (ø)
Core/include/Acts/TrackFitting/KalmanFitter.hpp 40.00% <75.00%> (ø)

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 a9ec279...734c632. Read the comment docs.

@gagnonlg
Copy link
Contributor

gagnonlg commented Mar 9, 2021

Hello @XiaocongAi , just a suggestion: std::unique_ptr would probably be safer here. https://en.cppreference.com/w/cpp/memory/unique_ptr

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.

Wow, looks like we spent a lot of time copying into containers, right?

As discussed: wherever we hit a performance wall, we should use bare pointers, etc.

Can you please specify though that the ownership of the container is NOT with the KalmanActor?

@asalzburger
Copy link
Contributor

Hello @XiaocongAi , just a suggestion: std::unique_ptr would probably be safer here. https://en.cppreference.com/w/cpp/memory/unique_ptr

Hi, I have had a similar comment bout, however, I am not sure a std::unique_ptr would work, as you get the inputMeasurements object and then just dereference it?

Let's try to do the following:

  • if there's really only one unique measurement container per KF/CKF run, then yes, we should try to make a unique_ptr.
    @XiaocongAi - can you clarify this, please?

@asalzburger asalzburger self-requested a review March 9, 2021 08:54
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.

We should try to clarify the ownership of the container.

@XiaocongAi
Copy link
Contributor Author

XiaocongAi commented Mar 9, 2021

Hello @XiaocongAi , just a suggestion: std::unique_ptr would probably be safer here. https://en.cppreference.com/w/cpp/memory/unique_ptr

Hi, I have had a similar comment bout, however, I am not sure a std::unique_ptr would work, as you get the inputMeasurements object and then just dereference it?

Let's try to do the following:

  • if there's really only one unique measurement container per KF/CKF run, then yes, we should try to make a unique_ptr.
    @XiaocongAi - can you clarify this, please?

Yes, the inputMeasurements is created per KF/CKF invocation. The KF/CKF fit/findTracks function has ownership of the inputMeasurements. The Kalman actor just deference it without any ownership of it. Since we know that the Kalman actor is created inside the fit/findTracks, i.e. the lifetime of the actor won't be longer than the inputMeasurements, I guess we should be safe with a bare pointer.

@asalzburger asalzburger self-requested a review March 9, 2021 16:21
@asalzburger
Copy link
Contributor

Merged 'master' in, when it's done, I will merge it in.

@asalzburger asalzburger merged commit 4b3509d into acts-project:master Mar 9, 2021
@paulgessinger paulgessinger modified the milestones: next, v7.0.0 Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants