Repository navigation
GH-45847: Pre-grow SwissTable to avoid repeated grow_double in map_new_keys - #53816
Asthenia0412 wants to merge 2 commits into
Conversation
|
|
…map_new_keys When GrouperFastImpl processes a minibatch where every row is a new key (common after a hash join scan emits rows with one unique key per payload), SwissTable::map_new_keys previously called grow_double() inside its do-while loop once per capacity threshold. Starting from log_blocks_=0 (8 slots), handling 1024 new keys required ~9 individual grow_double calls, each re-processing all existing entries, causing quadratic-ish overhead. This change pre-computes the required log_blocks capacity (mirroring num_groups_for_resize load factors: 50% for small tables, 75% for large) and performs all needed grow_double calls upfront when more than one is required. init_slot_ids_for_new_keys is called after the batched grow so slot ids are consistent with the new table size. All existing Grouper and hash aggregate tests still pass (83 row tests, 97 hash aggregate tests).
…apacity Previously the pre-grow loop broke when capacity >= new_total. But map_new_keys_helper triggers out_of_capacity when num_inserted_ == num_groups_limit (==), so capacity == new_total would still let map_new_keys_helper stop one entry short of the estimate, wasting the pre-grow effort. Use capacity > new_total to leave at least one slot of headroom. Also tighten the comments to remove an unimplemented claim about avoiding pre-grow on already-large tables.
8846c04 to
815e3d7
Compare
zanmato1984
left a comment
zanmato1984
left a comment
There was a problem hiding this comment.
The linked issue reports aggregation becoming exceptionally slow after a right outer join. This PR attributes that slowdown to repeated table growth, but the reproduction does not support that diagnosis.
I tested the original Acero plan using Arrow 26 static libraries, whose relevant baseline sources match this PR, rebuilding the SwissTable translation unit from both the base and head (815e3d7). All completed runs validated the resulting groups and counts.
- With one probe batch, 32 build batches of 32,768 rows, and four workers, the original plan took 39.67 seconds both before and after this change.
- Replaying 524,288 actual join-output rows through aggregation alone on one worker took 8.010 seconds before and 8.035 seconds after. Both versions performed 17 grows and exactly the same 1,219,618,330 slow-path block visits.
- In the baseline replay, lookup took approximately 7.27 seconds, while
grow_double()took approximately 0.096 seconds. Sorting or shuffling the same rows before aggregation reduced aggregation time to approximately 0.011–0.012 seconds, still with 17 grows; preparation time was excluded.
These results point to order-dependent probing costs, not resizing as the dominant cause of this workload's slowdown. Also, the implementation still loops over grow_double(); it does not perform a single target-sized allocation as described.
I do not think this should be merged as a fix for #45847. The root-cause analysis and proposed approach need to be revisited. If pre-growing is intended as a separate optimization, please provide a workload demonstrating a measurable benefit and adjust the description and issue-closing claim accordingly.
Separately, this investigation has identified substantial room to improve Fast Grouper's handling of these input orders, and I plan to work on that.
2 participants
Background
Issue #45847: Acero HashAggregate performs very poorly on multi-batch build scenarios (32+ batches). Performance drops 10x+ with only 1 CPU core utilized.
Root Cause
SwissTable::map_new_keysinkey_map_internal.ccstarts withlog_blocks_=0(8 slots) and callsgrow_double()each time capacity is exceeded. For N unique keys this causes O(log N) separate resize operations, each requiring O(N) re-insertion of ALL existing entries. With 2M unique keys from 64x32K batches, cascading resize dominates execution time.Solution
Pre-compute target capacity before the do-while loop and batch all grows at once:
log_blocks_so capacity >= num_inserted_ + num_idsnum_groups_for_resize)Changes: +44 lines, 0 deleted in
cpp/src/arrow/compute/key_map_internal.ccTesting
arrow-compute-row-test(Grouper): 83/83 PASSarrow-acero-hash-aggregate-test: 97/97 PASSarrow_compute_shared,arrow_acero_sharedcompile cleanlyFixes: #45847