From e933365cd96834a6aa0506e5e2d0e4533132b3d5 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 19 Oct 2026 09:32:08 -0400 Subject: [PATCH v2.1 6/6] Add inline fast paths for the Bloom filter checks in the probe loop The probe loop calls ExecHashBloomReject() and ExecHashBloomAdaptiveCreate() for every outer tuple. Both were out-of-line calls returning almost immediately in the common cases, so every hash join paid for two calls per outer tuple, even with enable_hashjoin_bloom = off. Split both functions into an inline part handling the cases that need no action, and an out-of-line part for the rest: * ExecHashBloomReject() becomes a static inline function, calling ExecHashBloomProbeFilter() only when there is a filter. * ExecHashBloomAdaptiveCreate() calls ExecHashBloomAdaptiveBuild() only when Bloom filters are enabled, there is no filter yet and the lookup completes a window. Inlining the whole functions would copy the cold build code into every call site. The wrapper is marked pg_always_inline, as gcc's partial inlining would otherwise move the window test out of line, and the slow path pg_noinline, to keep it out of the probe loop. Kept as a separate commit so that the effect can be benchmarked on its own. Quick timings show differences of a few percent either way, so a proper benchmark is still needed. --- src/backend/executor/nodeHash.c | 103 ++++++++++++++++------------ src/backend/executor/nodeHashjoin.c | 30 ++++++++ src/include/executor/nodeHash.h | 2 +- 3 files changed, 92 insertions(+), 43 deletions(-) diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 2e4c44fe801..50e0d445611 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -86,10 +86,10 @@ static void ExecParallelHashCloseBatchAccessors(HashJoinTable hashtable); static void ExecParallelHashBloomCreateFilter(HashJoinTable hashtable, int64 nelems); static void ExecParallelHashBloomBuildFilter(HashJoinTable hashtable); -/* FIXME would it make sense to mark some of these inline? */ static void ExecHashBloomCreateFilter(HashJoinTable hashtable, bool complete); static void ExecHashBloomBuildFilter(HashJoinTable hashtable, bool complete); -static void ExecHashBloomAdaptiveCreate(HashJoinTable hashtable); +static pg_always_inline void ExecHashBloomAdaptiveCreate(HashJoinTable hashtable); +static pg_noinline void ExecHashBloomAdaptiveBuild(HashJoinTable hashtable); static void ExecHashBloomSamplingUpdate(HashJoinTable hashtable, bool match); @@ -1076,33 +1076,23 @@ ExecHashBloomBuildFilter(HashJoinTable hashtable, bool complete) } /* - * ExecHashBloomReject - * Should this hash value (for an outer tuple) be rejected? + * ExecHashBloomProbeFilter + * Probe the Bloom filter for the hash value of an outer tuple. * - * Returns true if a Bloom filter is in use and it proves that the given hash - * value (and therefore the outer tuple) cannot match any inner tuple. + * This is the out-of-line part of ExecHashBloomReject() (see nodeHashjoin.c), + * called only when the hash table has a filter. Returns true if the filter + * proves that the hash value (and therefore the outer tuple) cannot match + * any inner tuple. * * When sampling the filter probes, most tuples bypass the filter and the * function returns false without consulting it. - * - * The caller is responsible for probing each outer tuple only once. Tuples - * written to a batch file have already been checked (either while processing - * batch 0 of a serial join, or while partitioning the outer relation of a - * Parallel Hash join), so there's no point in checking them again when they - * are read back. Doing so would waste cycles and distort the probe counters - * used to drive the adaptive sampling. - * - * XXX We can't mark it inline, it's called from a different translation unit. - * Maybe we could optimize it in some other way? */ bool -ExecHashBloomReject(HashJoinTable hashtable, uint32 hashvalue) +ExecHashBloomProbeFilter(HashJoinTable hashtable, uint32 hashvalue) { bool reject = false; - /* If there's no filter, all tuples should pass. */ - if (hashtable->bloomFilter == NULL) - return false; + Assert(hashtable->bloomFilter != NULL); /* * Probe the filter for the hash value, unless it should be skipped due to @@ -1178,19 +1168,21 @@ ExecHashBloomSamplingUpdate(HashJoinTable hashtable, bool match) * every BLOOM_BUILD_WINDOW lookups, build the filter if sufficiently few of * the lookups so far found a match. * - * XXX It's a bit unfortunate that when not inlined, we get to call this even - * with enable_hashjoin_bloom=off, only to bail out on bloomEnabled. So we pay - * for the call even when disabled, which may skew comparisons. Should we try - * to do better? Or is it not measurable. Same thing probably applies to the - * other ExecHashBloom methods. + * As this runs for every outer tuple, the cases that need no action - Bloom + * filters are disabled, the filter already exists, or the lookup does not + * complete a window - are handled here, inline. Only the lookup completing + * a window pays for the out-of-line call to ExecHashBloomAdaptiveBuild(), + * which makes the decision and builds the filter. + * + * We force the inlining: left to its own devices, gcc's partial inlining + * keeps only the first two tests inline and moves the window test into the + * out-of-line part, so that a join which never builds the filter (because + * the match rate is too high) would still pay for a call per outer tuple. */ -static void +static pg_always_inline void ExecHashBloomAdaptiveCreate(HashJoinTable hashtable) { - int64 hashLookups, - hashMatches; - - ParallelHashJoinState *pstate = hashtable->parallel_state; + int64 hashLookups; /* Bail out if Bloom filters are disabled. */ if (!hashtable->bloomEnabled) @@ -1203,32 +1195,59 @@ ExecHashBloomAdaptiveCreate(HashJoinTable hashtable) /* All batched runs should have a filter created automatically. */ Assert(hashtable->nbatch == 1); + /* + * Reconsider only once every BLOOM_BUILD_WINDOW lookups, counting the + * lookups already accumulated into the total too. + */ + hashLookups = hashtable->hashLookupsTotal + hashtable->hashLookups; + if (likely((hashLookups % BLOOM_BUILD_WINDOW) != 0)) + return; + + ExecHashBloomAdaptiveBuild(hashtable); +} + +/* + * ExecHashBloomAdaptiveBuild + * Build the Bloom filter if the join has been selective enough so far. + * + * This is the out-of-line part of ExecHashBloomAdaptiveCreate(), called at + * the end of each window of BLOOM_BUILD_WINDOW lookups while the hash table + * has no filter yet. If sufficiently few of the lookups so far found a + * match, build the filter from the tuples in the hash table (or, in a + * parallel join, make sure one participant does so). + * + * Marked pg_noinline so that none of this cold code ends up in the probe + * loop of the bucket-scan functions. + */ +static pg_noinline void +ExecHashBloomAdaptiveBuild(HashJoinTable hashtable) +{ + int64 hashLookups, + hashMatches; + + ParallelHashJoinState *pstate = hashtable->parallel_state; + + Assert(hashtable->bloomEnabled); + Assert(hashtable->bloomFilter == NULL); + /* use the total counts, including the not-yet-accumulated part */ hashLookups = hashtable->hashLookupsTotal + hashtable->hashLookups; hashMatches = hashtable->hashMatchesTotal + hashtable->hashMatches; + Assert((hashLookups % BLOOM_BUILD_WINDOW) == 0); + /* * Build a filter if the hash table lookups found sufficiently few matches - * so far. We recheck regularly, once every BLOOM_BUILD_WINDOW lookups, - * but we look at the whole lookup history. + * so far. We get here once every BLOOM_BUILD_WINDOW lookups, but we look + * at the whole lookup history. * * XXX Maybe we should reset the counters, just like we do for probes? * That would mean we look at individual windows instead, or some variant * (to eventually ignore very old lookups). Not sure if one of these is a * "more right". Looking at the whole history should not prevent creation * of a useful filter forever, but it can delay it of course. - * - * XXX If the modulo and multiplication turn out to be expensive, we could - * rewrite that to bitmasks. For example (hashLookups & 0x3FF) is fairly - * close to this, because 0x3FF is 1023. Not sure about the - * multiplication, though. But maybe the compilers already optimize that - * in some way (multiply-high?). */ - /* haven't collected enough probe samples yet */ - if ((hashLookups % BLOOM_BUILD_WINDOW) != 0) - return; - /* have enough samples, but there are too many matches */ if (hashMatches > hashLookups * BLOOM_BUILD_THRESHOLD) return; diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index fbf1132058b..cb80ffbc9cd 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -193,6 +193,8 @@ /* Returns true if doing null-fill on inner relation */ #define HJ_FILL_INNER(hjstate) ((hjstate)->hj_NullOuterTupleSlot != NULL) +static inline bool ExecHashBloomReject(HashJoinTable hashtable, + uint32 hashvalue); static TupleTableSlot *ExecHashJoinOuterGetTuple(PlanState *outerNode, HashJoinState *hjstate, uint32 *hashvalue, @@ -1138,6 +1140,34 @@ ExecEndHashJoin(HashJoinState *node) ExecEndNode(innerPlanState(node)); } +/* + * ExecHashBloomReject + * Should this hash value (for an outer tuple) be rejected? + * + * Returns true if a Bloom filter is in use and it proves that the given hash + * value (and therefore the outer tuple) cannot match any inner tuple. + * + * This is called for every outer tuple, so the common case of a hash table + * without a filter is handled here, inline, and only hash tables that do + * have a filter pay for the out-of-line call that probes it. + * + * The caller is responsible for probing each outer tuple only once. Tuples + * written to a batch file have already been checked (either while processing + * batch 0 of a serial join, or while partitioning the outer relation of a + * Parallel Hash join), so there's no point in checking them again when they + * are read back. Doing so would waste cycles and distort the probe counters + * used to drive the adaptive sampling. + */ +static inline bool +ExecHashBloomReject(HashJoinTable hashtable, uint32 hashvalue) +{ + /* If there's no filter, all tuples should pass. */ + if (hashtable->bloomFilter == NULL) + return false; + + return ExecHashBloomProbeFilter(hashtable, hashvalue); +} + /* * ExecHashJoinOuterGetTuple * diff --git a/src/include/executor/nodeHash.h b/src/include/executor/nodeHash.h index f236a02e5ae..5bf6e3247a4 100644 --- a/src/include/executor/nodeHash.h +++ b/src/include/executor/nodeHash.h @@ -37,7 +37,6 @@ extern void ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable, extern void ExecHashTableInsert(HashJoinTable hashtable, TupleTableSlot *slot, uint32 hashvalue); -extern bool ExecHashBloomReject(HashJoinTable hashtable, uint32 hashvalue); extern void ExecParallelHashTableInsert(HashJoinTable hashtable, TupleTableSlot *slot, uint32 hashvalue); @@ -76,5 +75,6 @@ extern void ExecHashBloomRememberFilter(HashJoinTable hashtable, bloom_filter *filter); extern void ExecHashAccumInstrumentation(HashInstrumentation *instrument, HashJoinTable hashtable); +extern bool ExecHashBloomProbeFilter(HashJoinTable hashtable, uint32 hashvalue); #endif /* NODEHASH_H */ -- 2.55.0