perf(vector): drop the two format! allocations from PayloadIndex::remove_field - #889
Merged
Merged
Conversation
…ove_field
remove_field built `{field}__lat` / `{field}__lon` scratch keys with two
`format!` calls, unconditionally, on EVERY call -- whether or not the
field (or even the document) was geo. This runs once per HSET field on
update_metadata_only's fast path (src/shard/spsc_handler.rs), which every
production reconcile after this deploy will drive hard.
`bytes::Bytes` implements `Borrow<[u8]>`, so the forward-index lookup
(`doc_values: HashMap<u32, HashMap<Bytes, DocFieldValues>>`) only ever
needed a `&[u8]`, never an owned `Bytes`. The candidate key is now built
once into a `SmallVec<[u8; 48]>` stack buffer and reused for both
suffixes; it only spills to the heap for a field name long enough to
exceed the inline capacity. `retire_values` takes `field: &[u8]` instead
of `&Bytes` for the same reason -- `Bytes: Borrow<[u8]>` covers every
existing call site via deref coercion.
One behavioral subtlety preserved on purpose: `insert_geo` derives its
sub-field prefix from `str::from_utf8(field).unwrap_or("")`, so a
non-UTF8 field name silently collapses to an empty prefix there. The
rewrite reproduces that exact fallback (`remove_field_retires_geo_
subfields_for_non_utf8_field_name`), or a non-UTF8 geo field's bitmap
entries would stop getting retired -- proven RED by mutating the
fallback out and confirming the new test fails on the pre-fix-shaped
code, then GREEN after restoring it.
Left untouched, with reasoning: `insert_geo` (line ~113) and the
GeoRadius arm of `evaluate_bitmap` (line ~347) have the same `format!`
pattern, but they only pay it when the data is actually geo -- insert_geo
runs once per geo field write, not once per field regardless of type,
and GeoRadius runs once per query, not once per key on the recovery
path. Fixing them was out of scope; they are not equally hot.
Measured on Linux/aarch64 (moon-bench-arm, release profile, fat LTO,
matching production): an insert_tag + remove_field cycle on a plain
non-geo field, 2M iterations, 8 interleaved runs per leg.
baseline (gh/main @ adf3a80): 747.8 ns/cycle, stdev 13.7 ns (1.8%)
fixed: 617.2 ns/cycle, stdev 10.4 ns (1.7%)
delta: 130.6 ns/cycle faster (17.5%), 9.5x the noise floor
A same-binary control (baseline run twice, same interleave pattern)
showed only 0.8% positional drift (6.1 ns), ruling out an ordering
artifact as the source of the 17.5% delta.
Tests: cargo test --release --lib vector::filter::payload_index (26
passed, 2 ignored measurement harnesses). Clippy clean with
-D warnings on both runtime-monoio (default) and runtime-tokio,jemalloc.
author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR rewrites geo sub-field lookup in ChangesGeo field retirement
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to This change reduces allocation overhead when removing geo fields while preserving geo-index retirement, including non-UTF8 field names. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
PayloadIndex::remove_fieldbuilt{field}__latand{field}__lonwith twoformat!allocations on every call — regardless of whether the field, oreven the document, was geo. It runs once per HASH field on
update_metadata_only's path (src/shard/spsc_handler.rs:4223), which is the.tpostfast path that every boot recovery drives hard.The change
bytes::Bytes: Borrow<[u8]>, so the forward-index lookup(
doc_values: HashMap<u32, HashMap<Bytes, DocFieldValues>>) only ever needed a&[u8]. The candidate key is now built once into aSmallVec<[u8; 48]>stackbuffer and reused for both suffixes by truncate+extend, never promoted to an
owned
Bytesunless the field name spills pastGEO_SUBFIELD_INLINE_CAP.retire_values'sfieldparameter moves from&Bytesto&[u8]; call sitesrely on deref coercion.
A subtlety preserved deliberately:
insert_geoderives its prefix viastr::from_utf8(field).unwrap_or(""), so a non-UTF8 field name collapses to anempty prefix there. The rewrite reproduces that exact fallback — dropping it
would leak a non-UTF8 geo field's bitmap entries on removal.
Red/green
New test
remove_field_retires_geo_subfields_for_non_utf8_field_name. The fixwas mutated on the bench host (UTF-8 fallback special case removed) and the test
went RED with a panic at the assertion; restoring the fix went GREEN.
26 tests pass in the module. Exit codes captured directly, not piped.
Measurement — Linux/aarch64, GCE
t2a-standard-8,--release(fat LTO)bench_remove_field_call_overhead: 2Minsert_tag+remove_fieldcycles on aplain non-geo field, 8 interleaved runs per leg. Baseline is
main(
adf3a808) with the same bench harness grafted in.130.6 ns/cycle, 17.5% faster, 9.5x the noise floor. A same-binary control
run through the identical interleave pattern showed 0.8% positional drift
(6.1 ns), ruling out an ordering artifact.
What this number is not. It is the call's own overhead in isolation, not
17.5% off a reconcile.
remove_field's remaining per-key cost isO(distinct terms in the document) for the term walk, which is inherent to
maintaining the index and is untouched here. Treat this as removing a constant,
not as a recovery-time claim.
Scope
insert_geo(payload_index.rs:~113) andevaluate_bitmap'sGeoRadiusarm(
~347) carry the sameformat!pattern and are left alone on purpose:insert_geoonly fires when a field actually is geo, andGeoRadiusruns onceper query rather than once per key on the recovery path. Neither is on the path
this PR targets.
Gates
cargo check --all-targetsclean on both runtime legs (monoio default, and--no-default-features --features runtime-tokio,jemalloc);cargo clippy --all-targets -- -D warningsclean on both;cargo test --release --lib vector::filter::payload_index::26 passed.Summary by CodeRabbit