Repository navigation
perf(rerank): select the best pool entries in one pass - #380
Closed
pavanmanishd wants to merge 2 commits into
Closed
pavanmanishd wants to merge 2 commits into
pavanmanishd wants to merge 2 commits into
Conversation
added 2 commits
October 20, 2026 14:14
The capped extract quickselects the rerank pool to 3x the cap and then sorts that prefix with qsort, one function-pointer comparison per step. A four-pass LSD radix sort over the float distances replaces it: stable, exact, and several times faster, taking roughly 40% off the posting phase at LIMIT 5000, where the prefix is about 95k entries. It uses a grow-only scratch buffer from the top-K's memory context. The key transform canonicalizes -0.0 to +0.0 so equal distances keep their arrival order instead of ranking one below the other, and sorts NaN last. One expected output moves: the extension suite pins a tie at LIMIT 3 on default_items, where the order is now deterministic but, after the capped path's quickselect, not necessarily arrival order. The figure above comes from a build with timescale#369 applied, which this branch does not contain: timescale#369 is approved and pending merge, and the two changes are independent, so this needs either the same base or a re-measurement before it lands.
The rerank computed exact distances for the whole pool and inserted each one into a threshold heap, then extracted the top k. At pool sizes of tens of thousands the per-candidate heap insert and its threshold check cost more than one selection at the end. Candidates are now appended to a buffer as they are scored, and a single stable radix sort selects the best k at the end (vs_topk_extract_best). The TID order the read stream walks comes from a six-pass radix sort over the encoded TID (vs_topk_sort_by_tid) instead of qsort with a comparator that decodes two ids per comparison, skipping a pass where every id agrees on that byte. The stream callback runs at look-ahead time, so scored distances are recorded by candidate index and appended in TID order after the stream ends: appending as each candidate is scored would order equal distances by whatever the stream happened to prefetch, and the selection keeps the append order among ties. That is what makes the tie order match the sequential scan's. One behavior change: the old path returned every candidate whose lower bound met the threshold, which with ties at the threshold could exceed keep; this returns exactly min(n, keep). vs_topk_reserve lets the rerank size its buffer once instead of growing through a series of reallocations.
Member
|
@pavanmanishd I cannot accept this patch without knowing more about the performance benefits of drawbacks. You need to illustrate that the patch clearly improves performance and that there are no regressions. It's easy to generate code using an LLM---I can do it myself. The hard part is ensuring the change moves the code base in the right direction. I don't have time to review or test random AI code changes. |
Author
|
@erimatnor Same context as my reply on #379. This targets the rerank's share of the profile in #384: the per-candidate threshold-heap insert (11.2% of query CPU at LIMIT 5000) and the TID-order sort (7.8%). It replaces both with one stable selection pass and a radix sort over encoded TIDs. On top of #379 it makes a single query about 8% faster, and 24-client throughput goes from 135 to 151 q/s with mean latency from 177 to 159 ms. Moved to draft while #384 is discussed. |
Member
|
FYI: #385 |
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
3 participants
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.
Refs #384.
Replaces the per-candidate threshold-heap insert in the rerank with an append and a single stable radix select at the end, and builds the TID order the read stream walks with a six-pass radix over the encoded TID instead of
qsortwith a comparator that decodes two ids per comparison.The stream callback runs at look-ahead time, so scored distances are recorded by candidate index and appended in TID order after the stream ends; appending as each candidate is scored would order equal distances by whatever the stream had prefetched. That, plus the stable selection, is what makes ties match a sequential scan. A byte that is uniform across every id is skipped, so the number of passes varies with the table and the sorted order is copied back whichever buffer the last pass landed in.
Two behavior changes worth naming: the old path returned every candidate whose lower bound met the threshold, which with ties could exceed k, where this returns exactly
min(n, k); and NaN exact distances are now kept and sorted last, where the heap insert dropped them. On a 24-client load the change is worth about +13% throughput and -28% p99.Contains #379's commit; review the top commit.
Disable-check: commit-count