Sd merge review fixes#6
Merged
Merged
Conversation
…dent oracle The old for_each_posting_matches_read_all_positions compared the decoder against read_all_positions, which now delegates to for_each_posting itself - a tautology that passed even with the position scratch-buffer bug (a missing positions.clear() leaks stale positions across docs, corrupting both sides identically). Replace it with a round-trip against independent ground truth: encode known postings via write_term_postings, decode via for_each_posting, assert equality. Varying per-doc position counts make a missing clear() fail loudly.
write_durable fsync'd file content but never the containing directory, so on POSIX the renamed directory entry (and the rename itself) was not durable across a power cut. Since optimize() collapses to one segment and deletes the old ones, losing the flip/dirent could leave the index unreadable. Add a best-effort, cfg-free sync_dir (real dir-fsync on Unix; File::open on a dir fails harmlessly on Windows, which relies on NTFS journaling), call it after write_durable's rename and after the segments.gen flip in write_optimized_generation, and correct write_durable's overstated doc comment. Incremental commit path left intentionally non-durable.
The near-stopword bounded-memory property (peak heap independent of a term's doc_freq) was only measured by the manual optimize_bench binary, not in CI - a regression to materializing whole terms would pass all tests. Add a tracking-allocator integration test that merges the same multi-segment input through both merge_segments_streaming and the batch merge_segments and asserts the streaming peak is >3x below the batch peak (measured ~6x). A global allocator in a tests/*.rs file only affects that test binary.
merge_streaming_inner wrote the final {merged_name}.cfs directly, so a
mid-write I/O error (disk full, etc.) left a truncated final .cfs on disk;
the cleanup only removed the .fdt/.frq/.prx temp files. Assemble into
{merged_name}.cfs.tmp, fsync, then atomic-rename onto the real .cfs and
fsync the directory, so a failure can only leave the staging file (now also
cleaned on both paths), never a partial final .cfs. Byte-identity tests
(which read _m.cfs) already cover rename correctness; added a regression
test that the staging file and final .cfs are clean when the rename fails.
A strict failing-first test for the mid-write I/O error itself needs a
fault-injection seam (test-only production code), so per 'TDD when possible'
this path is covered by the atomic-rename design + the reachable rename-fail
test rather than an injected mid-write error.
optimize() deleted the superseded segments' .cfs/.sti/.del immediately after the flip, but the prune deliberately KEEPS the previous generation's manifest as a grace window for lock-free readers. That manifest still referenced the just-deleted files, so a reader that opened the previous generation hit dangling references; on Windows the unlink of a reader-mapped .cfs also failed silently and leaked. Defer deletion one generation via a best-effort 'pending_deletions' sidecar: each flip reclaims the previous round's superseded files (whose referencing manifest the prune just removed) and records this round's for the next flip. A bug here can only leak disk, never delete a still-referenced file. Grace-window and reclamation covered by tests.
# Conflicts: # sdsearch-core/src/zsl/writer/index_writer.rs
This comment has been minimized.
This comment has been minimized.
…cene)
Measures rebuild, churn (delete 1% + add 1%), and search (many/few/none result
classes at top-20 and top-100 paging depths) across 1k–500k-document indexes,
comparing the PHP extension against Zend Search Lucene on an identical
deterministic corpus, plus a native Rust bench reporting the exact engine heap
that PHP's memory_get_peak_usage() cannot see.
- benches/run.sh: orchestrator — the {engine × size × workload} matrix, one
fresh process per measurement (clean peak-RSS), Zend timeout/address-space
guards, JSON collection, RESULTS.md render.
- tools/bench_compare.php: one measurement per invocation (sdsearch ext | Zend).
- tools/bench_report.php: renders RESULTS.md from results.jsonl.
- sdsearch-core/examples/bench_engine.rs: native bench, two passes (time pass =
honest ms/rss without the tracking allocator; heap pass = exact heap).
- benches/README.md, benches/RESULTS.md: docs + captured results.
Indexes are optimize()'d to production shape before churn/search. On the
captured run: rebuild 74–116× vs Zend, churn 5–30×, search (many results)
17–46×. The suite also surfaces a real characteristic — low-hit search reopen
cost scales O(N) (eager full term-dictionary load in ZslIndex::open) vs Zend's
lazy .tii seek, so sdsearch wins to ~100k but trails Zend on few/none at 500k.
…ndex, differential-tested)
…ed, incl. large corpus)
…ial-tested incl. large corpus)
…the whole .tis; eager kept as a test-only differential oracle)
…e-seek the term dict per term
Re-measured native + sdsearch (all sizes, 20 iters, fresh .so with the merge-cursor TermInfo fix). Story: - rebuild back to the pre-lazy baseline (native 500k 7.27s vs 7.22s; sdsearch 500k 9.84s vs 9.71s) — the ~40% optimize()/merge regression the lazy reader introduced is eliminated. - search few/none now BEAT Zend at scale (100k few 20.8x, 500k few 8.4x) where the eager reader lost 0.3-0.4x; many unchanged; churn improved. Zend rows are carried forward unchanged (engine untouched): 1k-100k verbatim from the prior run, 500k reconstructed from the committed RESULTS.md values (the report renders only ms/rss/p50/hits, all previously measured).
Contributor
Code Metrics Report
Details | | main (cea391d) | #6 (378dfae) | +/- |
|---------------------|----------------|--------------|-------|
+ | Coverage | 96.9% | 97.0% | +0.0% |
| Files | 36 | 36 | 0 |
| Lines | 5693 | 6200 | +507 |
+ | Covered | 5521 | 6017 | +496 |
- | Test Execution Time | 5s | 6s | +1s |Code coverage of files in pull request scope (96.5% → 96.7%)
Reported by octocov |
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
1 participant
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.
No description provided.