perf: stop re-walking the decided prefix at every node - #4
Draft
evanlinjin wants to merge 1 commit into
Draft
evanlinjin wants to merge 1 commit into
evanlinjin wants to merge 1 commit into
Conversation
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.
Draft. One commit, stacked on
experiment/ancestry-aware-seed— the branch behindbitcoindevkit#76, which is itself on
#75 and
#73. The base is set to that branch rather
than
master, so the diff here is only this change.Identical selections and identical round counts on all 42 benchmark fixtures. This is a pure cost
reduction, not a change of answer — which is the property that should make it cheap to review.
The problem
Branch and bound decides candidates in cursor order, so at a node with cursor c, every candidate at
position
0..cin the candidate order is already decided — selected by an inclusion frame, or bannedby an exclusion one. Two hot paths ignored that and walked the decided prefix at every node.
perfonwallet_mixed_2000, which spends its whole budget:LowestFee::boundBnbIter::exclusion_planMap::nth, called byexclusion_planOver 90% of the search was walking the candidate order. The visible symptom is a per-node cost
that grows with the pool:
The two fixes
exclusion_planre-walked the order. It resumed withcandidates().skip(cursor + 1).candidates()returns aMap, which has nonthoverride, soSkipadvances it one element at atime — O(cursor) per node. Added
candidates_from(from), which slices the order and iterates fromthere.
unselected()always started at the front. Every metric query about still-undecided candidatesfiltered through the decided prefix first.
best_undecided_value_pwuis the clearest case: it iswritten to scan only the run of candidates tying on the first undecided key, with a comment saying
the point is to make the query independent of pool size — and then reaching that first undecided
candidate cost O(depth) anyway.
SelectionViewnow carriesdecided_beforeand starts its scan there;BnbIterpasses its cursor.A
debug_asserton every view construction checks the promise, and the debug test suite exercisesit.
This is deliberately not a general cursor on
CoinSelector. Keeping it on the view means theclaim lives inside the search that maintains the invariant, rather than becoming something every
future caller of
select/ban/deselecthas to preserve.decided_beforeof 0 is alwayscorrect, so nothing outside branch and bound has to know it exists.
Measured
42 fixtures, 100,000-round budget, 5 runs after 2 warm-ups, median.
no_ancestry_2000wallet_mixed_2000shared_ancestry_2000no_ancestry_1000wallet_mixed_1000no_ancestry_500wallet_mixed_500shared_ancestry_1000Geometric mean 1.59x across all 42, median 1.43x.
The intended result shows in the shape more than the size: every budget-limited fixture now finishes
100,000 rounds in 21–25 ms regardless of pool size, where before it ran from 28 ms to 257 ms.
Per-node cost is flat in the pool.
A dead end, recorded so nobody repeats it
The first hypothesis was the greedy fill inside
LowestFee::bound, which re-derives a fundedselection from scratch at every node — genuinely O(pool) work, and the obvious thing to attack.
Stubbing the loop out (unsound, purely to price it) showed it is worth 5.3x on
no_ancestry_2000and 1.0x on everything else: every fixture with unconfirmed ancestors takes the
bound_with_ancestorspath, which never runs that loop. Worth pricing before building.Test plan
cargo test,cargo test --release,cargo clippy --all-featuresand a--no-default-featuresbuild all pass. The debug suite exercises the new
debug_assert. Behavioural equivalence is checkeddirectly: coinselect-benchmark compares
selections and round counts against the parent commit on all 42 fixtures and finds no difference.