Skip to content

Conversation

@stephenswat
Copy link
Member

Right now, we store only the top spacepoint in the device triplet which is technically all that is required, but this makes it very difficult and time-consuming to retrieve the bottom and middle spacepoint. This commit embeds the locations of those spacepoints in the triplet, making life easier for a lot of the planned seeding changes.

@stephenswat stephenswat added the refactor Change the structure of the code label Jul 28, 2025
@stephenswat
Copy link
Member Author

stephenswat commented Jul 28, 2025

This is necessary for e.g. #1082 and #1098.

@stephenswat
Copy link
Member Author

Performance summary

Here is a summary of the performance effects of this PR:

Graphical

Tabular

Kernel 327189d 5cfd02a Delta
fit_forward 51.48 ms 51.95 ms 0.9%
propagate_to_next_surface 42.43 ms 42.47 ms 0.1%
fit_backward 26.46 ms 26.54 ms 0.3%
count_triplets 8.34 ms 8.34 ms -0.0%
find_triplets 4.44 ms 4.43 ms -0.2%
find_tracks 3.73 ms 3.73 ms -0.0%
count_doublets 1.71 ms 1.73 ms 1.4%
find_doublets 1.27 ms 1.28 ms 0.8%
ccl_kernel 873.10 μs 871.52 μs -0.2%
Thrust::sort 456.34 μs 456.69 μs 0.1%
fit_prelude 410.71 μs 411.63 μs 0.2%
remove_duplicates 387.15 μs 390.74 μs 0.9%
select_seeds 345.85 μs 249.89 μs -27.7%
build_tracks 146.03 μs 146.93 μs 0.6%
update_triplet_weights 99.14 μs 105.05 μs 6.0%
apply_interaction 53.07 μs 54.09 μs 1.9%
fill_finding_propagation_sort_keys 35.05 μs 35.23 μs 0.5%
estimate_track_params 34.38 μs 34.34 μs -0.1%
populate_grid 30.38 μs 30.46 μs 0.3%
count_grid_capacities 29.12 μs 29.14 μs 0.1%
unknown 20.54 μs 20.58 μs 0.2%
fill_finding_duplicate_removal_sort_keys 18.61 μs 18.64 μs 0.1%
form_spacepoints 12.51 μs 12.56 μs 0.4%
reduce_triplet_counts 5.38 μs 5.37 μs -0.3%
fill_fitting_sort_keys 2.28 μs 2.26 μs -0.8%
make_barcode_sequence 1.01 μs 1.01 μs -0.6%
fill_prefix_sum 165.49 ns 165.53 ns 0.0%
Total 142.81 ms 143.35 ms 0.4%

Important

All metrics in this report are given as reciprocal throughput, not as wallclock runtime.

Note

This is an automated message produced on the explicit request of a human being.

@beomki-yeo
Copy link
Contributor

beomki-yeo commented Jul 30, 2025

The PR makes sense to me. Actually it is weird to me that only the top spacepoints has been saved in the past. Do you know why the top one is prioritized over the bottom one?

EDIT: NVM. I looked at the code and found that there is already a link to bottom and middle spacepoints

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

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

Looks good to me

Right now, we store only the top spacepoint in the device triplet which
is technically all that is required, but this makes it very difficult
and time-consuming to retrieve the bottom and middle spacepoint. This
commit embeds the locations of those spacepoints in the triplet, making
life easier for a lot of the planned seeding changes.
@stephenswat stephenswat force-pushed the feat/spbm_in_triplet branch from 5cfd02a to 0f6e85c Compare July 30, 2025 11:29
@sonarqubecloud
Copy link

@stephenswat stephenswat enabled auto-merge July 30, 2025 11:30
@krasznaa
Copy link
Member

The PR makes sense to me. Actually it is weird to me that only the top spacepoints has been saved in the past. Do you know why the top one is prioritized over the bottom one?

EDIT: NVM. I looked at the code and found that there is already a link to bottom and middle spacepoints

This was an aggressive optimization about 2 years ago. Reducing the memory allocations like this was making the seed finding on the TML detector faster at the time. It was also making seed finding appropriately fast for the ODD.

But now with the ITk all those optimizations seem to rather hurt us, as they are making it difficult to add new features to the code. As I discussed a couple of times with Stephen in the last week, to me this is a good warning that we must not go "too far" with optimizations "too early". (Whatever "too far" and "too early" may mean...)

@stephenswat stephenswat merged commit 9b5e684 into acts-project:main Jul 30, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Change the structure of the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants