Skip to content

tantivy: copy bundle ranges by slice, not byte by byte - #434

Merged
tonyalaribe merged 1 commit into
masterfrom
perf/bundle-read-copy
Oct 25, 2026
Merged

tonyalaribe merged 1 commit into
masterfrom
perf/bundle-read-copy

Conversation

@tonyalaribe

Copy link
Copy Markdown
Contributor

After #432 cut the indexes a warm 7-day errors list (6297304f) searches from 273 to 60, the query still spent ~3 s in the tantivy fan-out. It did not scale with hits (800 vs 200 hits both ~3 s; a window narrowed to today's small indexes: 32 ms) nor with the window range filter (day-aligned window still 2.4 s): it scales with the size of the day indexes in the window.

tantivy opens a fast-field column by reading it whole, and BundleDirectory::read assembled every range — hundreds of MB per day index, per query — through flat_map(|b| b.iter().copied().collect_vec()) with a vector per 64 KB block. It now copies each block's span with extend_from_slice.

Test: proptest a_bundle_range_reads_back_exactly (arbitrary ranges over block edges, cold and cached); red when the start offset is ignored.

Local signoff: make ci-signoff fmt/test/pg-smoke/e2e/clippy all pass (attested).

🤖 Generated with Claude Code

A warm 7-day errors list still spent ~3 s in the tantivy fan-out after the index
count fell 273 -> 60: tantivy opens a fast-field column by reading it whole, and
each bundle read assembled those hundreds of MB through a per-byte iterator with
a vector per block. Covered by a property test over block edges.
@monoscope-bot

monoscope-bot Bot commented Oct 25, 2026

Copy link
Copy Markdown

Monoscope production impact · Coverage unknown

Project 87576849-4941-49d3-a15d-680fef88a1a8 · reviewed 74a7e6791be964869f89b78bde03562d161d1ff4 · advisory

Evidence window (UTC): 2026-10-18 20:40:58.04257947 UTC → 2026-10-25 20:40:58.04257947 UTC

Coverage: Open Monoscope for evidence and coverage details.

Review history and settings

@claude

claude Bot commented Oct 25, 2026

Copy link
Copy Markdown

Review: LGTM.

  • Correctness: The new loop matches the old semantics. Only the first block is skipped, and the skip is clamped to the block length. The copy is capped at range.len() - out.len(), so it can't underflow. The Vec::with_capacity(range.len()) allocation is exact.
  • Performance: Dropping the per-64KB Vec and the byte-wise flat_map/skip/take is the right fix. It becomes a memcpy per block.
  • Tests: The proptest covers block edges, cold reads and cached reads. Two nits:
    • hi is always < len, so a range ending exactly at EOF (hi == len) is never generated. Use a.min(b)..=a.max(b) clamped to len, or % (len + 1) for the upper bound, to cover it.
    • (a.max(b) % len).max(a.min(b) % len) is hard to read. Compute lo and hi from a.min(b) and a.max(b), each reduced mod len + 1, then sort them.
  • Not checked: If any block is None (it shouldn't be after the fill loop), flatten silently yields a short read, same as before. An ensure!/debug assert on out.len() == range.len() would turn that into a loud error instead of corrupt data.

No security concerns.

@tonyalaribe
tonyalaribe merged commit 74a7e67 into master Oct 25, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant