shyjsarah commented on PR #82: URL: https://github.com/apache/paimon-vector-index/pull/82#issuecomment-5474866237
Re-reviewed the latest head (`540d672`). The previous blockers are fixed: scalar `find_topk` is now independent of the ambient Rayon pool width, the mixed-SGEMM test was corrected, and all CI checks are green. The latest PQ batch-size stability change also looks correct. I found one remaining performance blocker in the new batch worker policy (`core/src/kmeans.rs:96-109`, `1320-1336`): **Batch-size threshold causes a large top-k latency cliff.** `workers = min(nq, threads)` is also used to decide between linear selection and the bounded heap. With: ```text d=8, k=262144, nprobe=65536, Rayon threads=32 nq=4: median 2.69 ms nq=5: median 12.67 ms ``` Adding one query makes the batch about **4.7x slower**. At four workers the aggregate linear-selection scratch is exactly within `MAX_MATRIX_ELEMS`; at five workers it crosses the limit and every query switches to the much slower heap path. Please consider capping the number of concurrently executing linear-selection workers and processing excess queries in another wave, rather than changing the per-query algorithm based on batch width. This would keep the scratch bound without introducing the cliff. Minor follow-up: the scratch estimates still do not include stable-sort temporary allocation, so the documented 16 MiB scratch bound can be exceeded for large/full probes. So correctness and CI now look good, but I recommend addressing the batch performance cliff before merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
