[opt](function) Eliminate redundant hash computation in AggregateFunctionUniq#61730
Open
Mryange wants to merge 1 commit intoapache:masterfrom
Open
[opt](function) Eliminate redundant hash computation in AggregateFunctionUniq#61730Mryange wants to merge 1 commit intoapache:masterfrom
Mryange wants to merge 1 commit intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 26866 ms |
TPC-DS: Total hot run time: 169147 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
In
AggregateFunctionUniq::add_batch_single_placeandadd_batch(the hot pathfor
SELECT count(distinct col)), every key's hash is computed twice:set.prefetch(keys[i + HASH_MAP_PREFETCH_DIST])internally callsthis->hash(key)to locate the slot for CPU prefetch.set.insert(keys[i])goes throughemplace→EmplaceDecomposable→s.hash(key)again to find or prepare the insert position.The same codebase already has a correct "precompute hash + reuse" pattern in the
DistinctStreamingAggoperator path (hash_map_context.h), whereinit_hash_values()computes hash once, and subsequentprefetch/lazy_emplace_keycalls reuse the precomputed value. This PR applies the sameoptimization to
AggregateFunctionUniq.How does this PR solve it?
For both
add_batchandadd_batch_single_placeinaggregate_function_uniq.hand
aggregate_function_uniq_distribute_key.h:std::vector<size_t>before the main loop,using
set.hash(keys[i])— this is the only hash computation per key.set.prefetch(key)withset.prefetch_hash(hash_values[...])—reuses the precomputed hash, avoids recalculation and the unnecessary memory
access to
keys[i + HASH_MAP_PREFETCH_DIST]at prefetch time.set.insert(key)withset.emplace_with_hash(hash_values[i], key)— passes the precomputed hash directly, skipping the internal hash call.
Both
prefetch_hashandemplace_with_hashare existing APIs in phmap(
parallel_hashmap/phmap.h), no third-party changes needed.Expected improvements:
keys[], which is morecache-friendly than interleaving hash computation with hash-table probing
keys[i + HASH_MAP_PREFETCH_DIST]memory just to compute its hashRelease note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)