Repository navigation
[experimental] Add SINDI IP U12 + E5M7 storage - #1864
Conversation
|
@alexanderguzhva 🔍 Important: PR Classification Needed! For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:
For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”. Thanks for your efforts and contribution to the community!. |
418b139 to
b5d7af0
Compare
|
/kind improvement |
b5d7af0 to
81bb852
Compare
81bb852 to
2bba49e
Compare
|
@alexanderguzhva e2e jenkins job failed, comment |
|
wait for #1871 |
2bba49e to
4e3b7d9
Compare
39c91e9 to
0eefdde
Compare
|
|
||
| if constexpr (AllowIncremental) { | ||
| if (packed_ready_) { | ||
| expand_for_add(); |
There was a problem hiding this comment.
[P2] Keep growing-index additions incremental
Every subsequent packed Add unpacks all existing postings, then repacks the entire index while holding the CC write lock. Repeated small inserts therefore become quadratic. With 65,536 rows and 32 values per row, single-row additions averaged 52 ms for E5M7 versus 0.49 ms for FP16. Preserve an appendable representation or defer packing until sealing.
We can also simply disable this configuration (quant_type="e5m7") for growing indexes.
There was a problem hiding this comment.
added a needed speedup
| "Invalid ground-truth score order"); | ||
| previous = score; | ||
| if (id < 0) { | ||
| Check(base.rows.size() < k && j >= base.rows.size() && score == 0, "Invalid truth padding"); |
There was a problem hiding this comment.
[P2] Accept padding when sparse queries have fewer than k matches
Padding is now accepted only when the entire corpus contains fewer than k documents. Sparse queries can have fewer than k positive matches in a larger corpus. A reproduction with 100 documents, k=10, and one match aborts with Invalid truth padding. Accept trailing sentinels independently of corpus size.
| } | ||
|
|
||
| if (refine_) { | ||
| rebuild_refinement_seek(); |
There was a problem hiding this comment.
There is a similar pre-existing issue with refine=true, introduced in #1856. Even with quant_type="fp16", every incremental Add calls rebuild_refinement_seek(), which rebuilds the seek metadata for all dimensions/windows and validates every existing posting while the CC write lock is held. Repeated small inserts therefore have quadratic cumulative cost and block concurrent searches during each rebuild.
In a local test on this PR with 65,536 existing rows and 32 nonzeros per row, 20 single-row additions averaged 14.29 ms with refine=true versus 0.30 ms with refine=false. We can also disable refine=true for growing indexes until the seek metadata is maintained incrementally and validation during Add is limited to new postings.
| std::vector<uint8_t> ids(sindi::packed12_bytes(count), 0), vals(ids.size(), 0); | ||
| max_scores_per_dim_.assign(this->nr_inner_dims_, 0); | ||
| // Sequential pair ownership avoids shared-nibble races at term boundaries. | ||
| for (size_t dim = 0; dim < this->nr_inner_dims_; ++dim) { |
There was a problem hiding this comment.
Non-blocking optimization suggestion; we can leave this to subsequent PRs.
On the full msmarco_splade dataset (8,841,823 rows), with 8 build workers and refine=false, U12+E5M7 took 28.66 s to build versus 10.22 s for U16+FP16. The existing log timestamps suggest roughly 10.3 s in the serial validation/counting phase before parallel construction and 7.6 s in the final packing phase (approximate phase timings, including nearby setup/cleanup).
We could merge the validation and posting-count checks into one parallel row scan, potentially reusing scan_rows_for_build(). For packing, partition work by complete pairs of 12-bit entries (3 bytes per pair in each buffer), rather than by dimension, so workers never share an output byte; handle the odd tail separately. This would allow parallel packing without changing the storage format. The sealed-build path could also reuse the already-computed maxima of the quantized values instead of recomputing max_scores_per_dim_ here.
There was a problem hiding this comment.
will do in a follow up, if you don't mind. The candidate is available
There was a problem hiding this comment.
no problem, we can improve the build time/build memory usage in the next prs
ca5ee8d to
037a350
Compare
Signed-off-by: Alexandr Guzhva <alexanderguzhva@gmail.com>
037a350 to
b8b23b5
Compare
|
issue: #1903 |
| if constexpr (is_bm25_u8) { | ||
| if constexpr (is_ip) { | ||
| const size_t count = plists_dim_offsets_span_.back(); | ||
| std::vector<uint8_t> ids(sindi::packed12_bytes(count), 0), vals(ids.size(), 0); |
There was a problem hiding this comment.
Non-blocking follow-up: reduce peak memory during sealed packing. We can leave this to a subsequent PR.
The complete packed output is allocated while the typed posting arrays are still retained. For N postings, U12+E5M7 temporarily holds about 4N bytes of U16/FP16 input plus 3N bytes of packed output (7N total, versus 3N after packing). The existing BM25 U4 path has the same overlap: about 5N bytes for U12+U4 or 5.5N for U16+U4, and it also builds temporary seek metadata before releasing the typed arrays. These are posting-buffer estimates, excluding input vectors and other metadata, not measured peak RSS.
A follow-up could construct the final packed buffers directly once sizing and codec setup are known (BM25 needs the histogram/LUT pass first), or use a construction layout that allows bounded chunks to be converted and their source storage released incrementally. As a smaller improvement, BM25 could release its typed posting arrays immediately after packing, before building the temporary seek metadata.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexanderguzhva, chasingegg The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is an experimental PR, which tests U12 instead of U16 and E5M7 instead of FP16 for SINDI IP. It trades the QPS & recall for the smaller database size (~25% less for MSMARCO / SPLADE).
This PR is based on top of #1856, so the refinement is also enabled.
The full results are https://zilliverse.feishu.cn/wiki/KLwZw50Lmi0zCYk1R0pctgp5nug?sheet=LNmWIz
Some short results for M7i 8c32b machine with
clang-22compiler: