Skip to content

Conversation

@aalkin
Copy link
Member

@aalkin aalkin commented Aug 11, 2025

Original algorithm was performing extremely poorly for grouping by very large (in terms of number of rows) tables, typical for the derived data, due to being O(N^2). Precalculating the offset cache at the start solves this issue, dramatically improving the performance for the extreme cases.

@ddobrigk

@aalkin aalkin requested a review from a team as a code owner August 11, 2025 11:32
@github-actions
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0
async-2024-PbPb-apass1
async-2024-ppRef-apass1
async-2024-PbPb-apass2
async-2023-PbPb-apass5

@aalkin aalkin force-pushed the improve-grouping-performance branch from 0d219a5 to 0315fa1 Compare August 12, 2025 06:26
@ktf
Copy link
Member

ktf commented Aug 14, 2025

I have tested this with Correlations on hyperloop (which I understand uses grouping) and it seems to. I do not see any particular change in performance though.

@ddobrigk Could you comment on wether your tests improved? I will then merge.

@ktf
Copy link
Member

ktf commented Aug 14, 2025

@ddobrigk you can use my tag on hyperloop (eulisse-local) to have a build with the PR included.

@ddobrigk
Copy link
Contributor

Hi @ktf yes good point. Let me test right away, will write once I have results !

@aalkin
Copy link
Member Author

aalkin commented Aug 14, 2025

To clarify: the performance in the usual cases - grouping of tracks over collisions in normal AODs - should be unchanged. The performance impact will be quite noticeable in derived data that packs a lot of entries in the tables used for grouping.

@ddobrigk
Copy link
Contributor

To clarify: the performance in the usual cases - grouping of tracks over collisions in normal AODs - should be unchanged. The performance impact will be quite noticeable in derived data that packs a lot of entries in the tables used for grouping.

Yes, sure. I will run the same synthetic tests in which I vary the number of elements to group from an average of 1e-2 to 100 elements per "collision", so that we can directly gauge. The main problem is I screwed something up and am now stuck recompiling (sorry). More soon ...

@ddobrigk
Copy link
Contributor

ddobrigk commented Aug 14, 2025

@aalkin @ktf I finished running a first set of benchmarks with a 'synthetic' AO2D scenario in which I have multiple test sizes for elements being grouped to collisions, from an average of 1e-2 elements per collision to 10^4 elements per collision.

This PR helps a lot already: see the benchmark result below, with the improved version gaining, say, up to an order of magnitude in the (very common!) range around 1-10 elements (e.g. tracks, V0s, etc) grouped per grouping entity (e.g. collision): it is much improved! :-) For reference, the 'custom' grouping option corresponds to this code (very simple, very naive, and capable of dealing with the unsorted case). If you like, I can also explore other scenarios (fixed DF size at X megabytes, etc) for testing: just let me know :-)

Screenshot 2025-08-14 at 22 56 19

@ktf ktf enabled auto-merge (squash) August 14, 2025 21:18
@ktf ktf disabled auto-merge August 14, 2025 21:18
@ktf ktf enabled auto-merge (squash) August 14, 2025 21:18
@ktf ktf merged commit 65275d9 into AliceO2Group:dev Aug 14, 2025
11 checks passed
mhemmer-cern pushed a commit to mhemmer-cern/AliceO2 that referenced this pull request Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants