acts-project / traccc

Demonstrator tracking chain on accelerators
Mozilla Public License 2.0
29 stars 50 forks source link

exp: propagation-centric host ckf #761

Open niermann999 opened 4 weeks ago

niermann999 commented 4 weeks ago

This is just a test to see what would need to be modified, in order to keep the navigation state alive and use candaidate caching in the navigator. So far, it only holds the 256 byte navigation state in addition to the bound track parameters and not the entire propagation state. This means, however, that the track parameters need to be copied in and out of the propagation state at every step and for every branch. This ckf implementation needs some additional changes in detray before it will compile.

Additionally, it sorts the valid measurements at every step, so that the best matching measurements get assign to the original branch and any new branches, in case the number of branches is limited to e.g. one or two.

The memory handling and the way the measurements are traces is not particularly smart, as that was not the focus of this example PR

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

beomki-yeo commented 3 weeks ago

I would be onboard with the idea of candidate caching, but this PR seems to be mixture of several independent changes. It would be good to split this PR

And I suggest to modify find_tracks.hpp instead of making find_trakcs_alt.hpp

niermann999 commented 3 weeks ago

I would be onboard with the idea of candidate caching, but this PR seems to be mixture of several independent changes. It would be good to split this PR

And I suggest to modify find_tracks.hpp instead of making find_trakcs_alt.hpp

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator? If I should port something from this PR into find_tracks.hpp I can do that of course (e.g. the measurement range finding?). Some of these changes actually belong to #714 and #763

beomki-yeo commented 3 weeks ago

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator?

What do you mean? so are you going to add find_tracks_alt.hpp anyway?

niermann999 commented 3 weeks ago

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator?

What do you mean? so are you going to add find_tracks_alt.hpp anyway?

That was my intention, yes. I would like to compare, if the additional memory consumption by the caching is possibly eating up the benefit from the more efficient navigation. But in order to make that comparison, we need to remove the additional work for the navigation cache from find_tracks.hpp. Does that make sense?

beomki-yeo commented 3 weeks ago

No. I don't think that justifies your intention. you can compare the performance between main branch and your PR. Continuous benchmark was developed in the same spirit.

But in order to make that comparison, we need to remove the additional work for the navigation cache from find_tracks.hpp

What is the additional work and the navigation cache from find_tracks.hpp?

niermann999 commented 3 weeks ago

What is the additional work and the navigation cache from find_tracks.hpp?

It needs to hold more memory to cache candidates that it never uses (per branch) and we have seen that memory consumption makes a difference in performance. It also needs to build the cache and that means an additional step of sorting the candidate array every time a sensitive surface is reached. It also needs to resolve the trust levels and different update schemes of the cache (e.g. it rebuilds the entire cache if the next candidate is not reachable). That means a layer of of if-else constructs. All of this complexity is not needed in find_tracks.hpp and might slow it down, leading to an incorrect comparison imo

beomki-yeo commented 3 weeks ago

If I understood correctly, you want to compare the performance find_tracks.hpp and find_tracks_alt.hpp and to do this in correct way you also need to fix find_tracks.hpp?

May I suggest to separate this into two sequential steps rather doing this in one-go?

  1. Update find_tracks.hpp for the fair comparison (corresponding to your last comment) and see if performance improves
  2. Update with candidate caching and see if performance improves
beomki-yeo commented 3 weeks ago

Then we don't have to make two parallel implementations for find_tracks.hpp.