Fix race check failures in shared memory groupby

[PointKernel]

Description

Replace #17976

This fixes the race check failures in shared memory groupby and resolves NVIDIA/spark-rapids#11835.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[PointKernel]

Missing this block sync is the culprit of the Spark query failure and I still don't understand why

[bdice]

Is it equivalent if you put the block.sync(); after compute_final_aggregations? I think that would be better. When entering this while loop, we should already be synced by line 211. Syncing at the end of the loop would feel safer, since it forces a final sync that won't happen if the sync is at the beginning of this while loop.

[PointKernel]

Is it equivalent if you put the block.sync(); after compute_final_aggregations?

No, the current implementation is actually synced after compute_final_aggregations (or at the end of thecompute_final_aggregations) and failed the spark query. That's why I'm saying I don't understand 😕

[PointKernel]

More info for reference

If we sync at the end of the loop right after compute_final_aggregations. compute sanitizer will report a race condition between

cudf/cpp/src/groupby/hash/compute_shared_memory_aggs.cu

Line 73 in 847fa28

++col_end;

and

cudf/cpp/src/groupby/hash/compute_shared_memory_aggs.cu

Line 215 in 847fa28

while (col_end < num_cols) {

.

Relocating the block sync from the end of the loop to the current position eliminates the race check failure and resolves the Spark query issue.

[bdice]

Does this explain it?

1. We're at the beginning of the while loop.
2. All threads read the shared variable col_end to compare col_end < num_cols.
3. We are adding a block sync here!
4. Thread 0 runs calculate_columns_to_aggregate and updates the shared variable col_end.

Other threads besides thread 0 could have a race condition between (2) and (4), unless we force them to be ordered by inserting a sync at (3).

[pmattione-nvidia]

These functions are not inlined, so I can see how maybe moving the block sync could change the timing such that the bug is now masked. But I think it may still be there hiding. I wouldn't be surprised at all if this was a false positive on racecheck. Or this change is just lucky for it.

[PointKernel]

Other threads besides thread 0 could have a race condition between (2) and (4), unless we force them to be ordered by inserting a sync at (3).

Totally makes sense. 🔥

[pmattione-nvidia]

Ah yes, that's it @bdice

[PointKernel]

These functions are not inlined, so I can see how maybe moving the block sync could change the timing such that the bug is now masked. But I think it may still be there hiding. I wouldn't be surprised at all if this was a false positive on racecheck. Or this change is just lucky for it.

I see your concern. To be honest, I still don’t understand how the current change resolves the Spark failure. However, if it were an illegal memory access issue, our nightly memcheck tests should have already detected it, and our Python and C++ tests wouldn’t have survived thousands of runs.