lookup: ask whether a comparison reads the keys, not whether == is enough - #165
Open
psiha wants to merge 1 commit into
Open
lookup: ask whether a comparison reads the keys, not whether == is enough#165psiha wants to merge 1 commit into
psiha wants to merge 1 commit into
Conversation
psiha
marked this pull request as draft
September 11, 2026 08:35
psiha
force-pushed
the
bt/10-comparator-cost
branch
from
September 11, 2026 08:37
16ab6ff to
7c4c9fc
Compare
psiha
marked this pull request as ready for review
September 11, 2026 21:10
…ough is_simple_comparator answers a semantic question - may == replace the double-negation equivalence test - and linear_search_eligible reads it as a cost one. They are different properties, and a common comparator shape separates them: a key that is a handle, ordered by fetching what it points at. Such a comparator is exactly as "simple" while every comparison is a scattered load instead of a read of the cache line the scan is already walking, so a trait that says nothing about locality was selecting the strategy whose whole economics is locality. So state the cost question separately, as is_direct_comparator, and have linear_search_eligible require both. Directness is a CLAIM rather than an assumption: a comparator that says nothing gets std::lower_bound. The opposite default would need a projecting comparator to opt out, which leaves the party that knows least - one that says nothing at all - inheriting the strategy that suits it least. std::less/greater, the ranges forms and the erasure adapters name the key type's own ordering, so they read the keys and nothing else, and are declared direct. Every comparator that reaches the linear path in library code does so through one of those, so this changes the behaviour of no existing code: it makes the conflation un-reintroducible rather than fixing a live miscompile. The benchmark's indirect_less keeps its is_simple_comparator specialisation, which is accurate, and declares itself non-direct. PSI_VM_BENCH_INDIRECT_LINEAR=1 claims the directness it does not have, which is the only route back to the scan and the reason that arm is a knob rather than a value. Measured, for whenever the default does start to bite: against that comparator at 512-byte nodes the two strategies trade places by ~14% in opposite directions - binary ahead on lookup, the scan ahead on insert - both about nine times the in-run noise floor. AArch64 agrees on lookup (21%) and is mixed on insert. Full suite: 2078 passed, 1 pre-existing skip, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
psiha
force-pushed
the
bt/10-comparator-cost
branch
from
September 11, 2026 21:11
f044dad to
32784a2
Compare
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.
Stacked on
bt/9-bench-rigor(#163).The conflation
is_simple_comparatoranswers a semantic question — may==replace thedouble-negation equivalence test — and
linear_search_eligiblewas reading it as acost one. They are different properties, and a common comparator shape separates
them: a key that is a handle, ordered by fetching what it points at. Such a
comparator is exactly as "simple" while every comparison is a scattered load
rather than a read of the cache line a scan is already walking. So a trait that says
nothing about locality was selecting the strategy whose whole economics is locality.
The change
Separate the cost question as
is_direct_comparatorand havelinear_search_eligiblerequire both.
The default is
false, so a comparator that does not claim directness gets thebinary search. That is the right way round for a reason beyond the measurement: the
opposite default would need a projecting comparator to be opted out, and the
opt-out would have to be spelled
is_direct_comparator<indirect_less>{ false }— whilethe dangerous case, a consumer silently inheriting the wrong strategy, would be the
one that says nothing. Making directness a claim rather than an assumption puts the
burden on the party that knows.
std::less/std::greater, therangesforms and the erasure adapters name the keytype's own ordering, so they read the keys and nothing else — those are direct.
Worth stating plainly, because the diff looks like it flips a hot-path policy:
built-in specialisations above, and all of them are now declared direct. Unchanged.
komparator.hppis intest/b+tree.cpp, on the benchmark'sindirect_less. Test-only.is_simple_comparatorfor a projectingcomparator was rama, and that specialisation has just been deleted on its side.
So this lands as a guard and a piece of documentation: it makes the conflation
un-reintroducible rather than fixing a live miscompile. The measurement below is why
the default is also the right one for whenever it does start to bite.
What the measurement says, for the record
Arms differ only in the indirect comparator's search. 4 measured blocks per arm after
a discarded warmup, arm order reversed on alternate blocks, plus a same-config
duplicate arm as an in-run noise floor.
x86-64 (Arrow Lake-H), 512-byte nodes — noise floor 1.53%:
INDIRECT_LINEAR=1)Binary 13.9% faster on lookup, linear 14.6% faster on insert — both ~9× the
noise floor, so it is a genuine operation-dependent trade rather than a win.
AArch64 (M1) agrees on lookup (21% at both 1024- and 2048-byte nodes) and is
mixed on insert.
indirect comparator at 4096 is not evidence for a binary default. That is
PSI_VM_BT_RUNTIME_DISPATCHon vs off — dispatching on a node's actual fillinstead of its capacity — and at 4096 the capacity-based choice is already binary, so
the win there is the scan arriving on under-filled nodes. It measures a different
mechanism and points the other way.
The benchmark's arm
indirect_lesskeeps itsis_simple_comparatorspecialisation (it genuinely issimple) and declares itself non-direct, so it takes the binary search by default.
-DPSI_VM_BENCH_INDIRECT_LINEAR=1claims the directness it does not have — the onlyroute back to the scan, and the reason that arm is a knob rather than a value.
Follow-up, not part of this PR
PSI_VM_BT_RUNTIME_DISPATCHis not in this diff — it arrives in #163(
b+tree/base.hpp:63, consumed atimpl.hpp:306and:357, default off). Given the4096 figure above, the question worth asking next is whether
is_direct_comparatorshould feed that — i.e. decide when per-call fill dispatch is worth paying for —
rather than only the compile-time predicate. Separate change, separate measurement.
🤖 Generated with Claude Code