perf: reduce large-document binary Ion 1.0 read costs on the Element and AnyEncoding paths - #1040
Draft
hsbakshi wants to merge 6 commits into
Draft
perf: reduce large-document binary Ion 1.0 read costs on the Element and AnyEncoding paths#1040hsbakshi wants to merge 6 commits into
hsbakshi wants to merge 6 commits into
Conversation
The existing benchmarks read multi-megabyte streams through a single reader, which amortizes away both per-document fixed costs and mid-size scanning behavior. This benchmark measures the one-reader-per-document pattern on ~10KB and ~30KB binary Ion 1.0 documents in two flavors: value-heavy (a small set of field names reused across many structs) and symbol-heavy (every field name and symbol value distinct, so the local symbol table grows with the document). Cases cover the stable Element::read_one API, by-name struct field lookups on a materialized document, and (behind the existing experimental-reader-writer feature, like the other benchmarks) the streaming Reader with both AnyEncoding and the concrete v1_0::Binary encoding.
When a parsed local symbol table is applied, the number of incoming symbols is already known, but the symbol table's backing Vec and text map grew incrementally as each symbol was added. For symbol tables with many entries this caused repeated reallocation and rehashing. Reserve capacity for all pending symbols before adding them.
Struct's by-name field index used the standard library HashMap with its default SipHash hasher, which showed up prominently in profiles of DOM-materialization-heavy workloads. Swap it for FxHashMap, which the crate already uses for the reader's symbol table text index. The index type is internal to Struct and does not appear in any public signature.
The AnyEncoding wrapper types in any_encoding.rs consist almost entirely of single-match dispatch methods and From conversions that wrap or unwrap one enum layer. The same is true of RawValueRef's resolve and expect_* methods in raw_value_ref.rs. None of these were candidates for cross-crate inlining without an #[inline] hint, so every value visited through Reader::new(AnyEncoding, ...) paid call overhead several times per value. Mark them #[inline]. Additionally, declare the Binary_1_0 variant first in the wrapper enums on the hottest read path (LazyRawValueKind, LazyRawFieldNameKind, RawAnyStructIteratorKind) so the most common encoding's variant is the zero discriminant, which dispatches most cheaply. Measured together on ~10KB/~30KB binary Ion 1.0 documents, these reduce AnyEncoding read-all time by roughly 12-13% relative to the prior baseline.
- struct.rs: hand-write Debug for Fields to print only by_index, so output does not depend on hash map iteration order; document the FxHashMap choice (same untrusted symbol-text input that symbol_table.rs already indexes with FxHashMap ids_by_text). - large_doc_read bench: add ~15% absent field names to the struct_get_by_name lookups so the index's miss path is exercised; keys are still materialized outside the timed loop. - raw_value_ref.rs: drop #[inline] from Debug::fmt (13-arm formatting fn, never hot) and the expect_* family (error arms contain format!, which inlining pulls into callers); keep it on the hot eq/resolve paths.
- trim the #[inline] tranche: keep the annotation on small single-match dispatch methods and From conversion shims, remove it from large multi-branch functions (next, resume, save_state, from_value, the LazyRawStreamItem From impls) and from generic functions (RawValueRef::resolve, PartialEq::eq); document the convention at module level in any_encoding.rs - reword the variant-order comments to drop the zero-discriminant codegen claim; the ordering is justified by benchmark measurements with no compiler layout guarantee assumed - reserve ids_by_text capacity only for symbols with known text; symbols with unknown text never get a text-to-ID entry - benches: replace stringly-typed flavor dispatch with a Flavor enum and move criterion group names to slash-path style (large_doc_read/<benchmark>/<flavor>_<size>)
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
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.
Summary
This PR reduces read costs for large (~10–30 KB) binary Ion 1.0 documents
on the
Element/DOM path (the API that parses a whole document into anin-memory tree) and the lazy
AnyEncodingpath (the reader mode thatauto-detects text vs binary), and adds a large-document benchmark
covering them. The optimizations are independent: reserving
symbol-table capacity when applying a local symbol table, an
FxHashMapfield-name index on
Struct, and an inlining/variant-ordering pass overthe
AnyEncodingdispatch layer. On value-heavy documents,Element::read_oneimproves 10–13% on the criterion benchmark (7–8%under an independent profiling harness; both aarch64),
Struct::getbyname improves
30–37%, and the lazy
AnyEncodingreader improves 9–12%. No public APIchanges. Follows discussion in #1038.
Motivation
Large documents spend their time in different places than small ones.
Profiling large binary Ion 1.0 documents (value-heavy and symbol-heavy
flavors at ~10 KB and ~30 KB, one reader per document) shows:
Element::read_onecosts ~3.8xthe concrete lazy reader (
v1_0::Binary) reading the same bytes.AnyEncodinglayer adds +48–49% overv1_0::Binaryonvalue-heavy documents at both sizes. The gap grows with document size,
so large documents do not amortize it away.
Struct's by-name field index shows ~12% ofElement-path profile samples inside SipHash hashing frames (SipHashis the DoS-resistant hasher Rust's
HashMapuses by default).capacity reservation, so symbol-heavy documents pay repeated map/vec
growth.
The existing benchmark suite has no large-document
one-reader-per-document case and no by-name field lookup case, so none of
these paths had a regression guard.
What changed
b6cfb07perf: add large-document read benchmark: newbenches/large_doc_read.rs. Value-heavy and symbol-heavy documents at~10 KB and ~30 KB;
Element::read_one(default features); lazyAnyEncodingandv1_0::Binaryreaders (feature-gated like theexisting benches); and a
Struct::getby-name lookup flavor thatincludes ~13–17% miss lookups, so both hit and miss paths are guarded.
6d6e61fperf: reserve symbol table capacity when applying a localsymbol table: reserves capacity up front from the parsed
pending-symbol count at LST-apply time. (The count is only known after
parsing: the binary symbols-list length prefix is in bytes, not
elements.) Removes growth-reallocation churn on symbol-heavy documents.
4f0bcfdperf: use FxHashMap for Struct's by-name field index:swaps the index
HashMaptoFxHashMap(a much faster hasher that isnot DoS-resistant). Rationale: field names are the same untrusted-input
class as symbol text, and the existing
ids_by_textindex insymbol_table.rsalready usesFxHashMapfor that. The exposureprofiles do differ: the symbol table precedent is per-stream and
transient, while
Structis a retained DOM value; that difference ispart of why the commit is severable. The hasher does not appear in any
public signature.
Struct'sDebugoutput changes: the field-nameindex is no longer printed (it is derived data with arbitrary map
ordering). Note for maintainers: if you prefer
DoS-resistant default hashing on DOM types (the
serde_json::Mapconvention), we're happy to drop this commit; it's severable.
3a20f19perf: inline AnyEncoding delegation and reorder hotdispatch enums: adds
#[inline]to ~150 smallAnyEncodingconversion/delegation functions, letting the compiler eliminate call
overhead in this thin wrapper layer, and declares the
Binary_1_0variants first in three internal-dispatch enums so the hot variant is
matched first. Source-compatible: the three enums derive only
Debug/Copy/Clone, with noOrd, no explicitrepr, and nodiscriminant reads (verified).
Debug::fmtand theexpect_*error-path functionswere deliberately excluded from inlining. The annotations were
measured in aggregate via the new benchmark, not individually; the win
survived the trim in the review commit (−10 to −13% re-measured).
ea09fc7review polish: naming, comments, and bench cleanup fromself-review.
bd3eb06refactor: apply local review feedback: trims the#[inline]set to small dispatch/Fromshims and documents thatconvention at the module level; comment corrections; the capacity
reservation now counts only symbols that enter the text index.
Benchmarks
Median of 3 interleaved before/after runs (aarch64, release; baseline =
mainplus the new benchmark commit; x86_64 run to follow before merge;see caveat below). Values shown as 10 KB / 30 KB:
element_read_one(value-heavy)struct_get_by_name(incl. miss lookups)lazy_any_read_all(value-heavy)lazy_binary_1_0(value-heavy)lazy_binary_1_0(symbol-heavy)The concrete
v1_0::Binaryvalue-heavy case is unchanged, as expected:none of the three optimizations touches that path. Its symbol-heavy
improvement comes from the capacity reservation, which runs on every
path.
Existing suite, before/after:
read_many_structsandencoding_primitives, 35 cases total, no sustained change >2%.We also cross-checked the results with a second, independent profiler
(dial9-tokio-telemetry,
perf-based CPU sampling). By-name lookups reproduced at −42 to −44%, with
the baseline SipHash frames disappearing from the profile entirely.
Element::read_onereproduced directionally at −7 to −8% under thatprofiler's harness. Output checksums were identical between baseline and
patched builds on every run.
Severability
The three optimizations are independent: each commit stands alone,
individually measurable and individually revertable. Happy to split any
of them out (in particular the
FxHashMapcommit, per the hashing noteabove) or reshape scope on request.
Relationship to the small-document PR
Complements our small-document fixed-cost PR (#1039). That PR targets per-document setup costs
(reader construction, IVM/LST processing); this one targets the
value-scanning and DOM costs that dominate on large documents. The
branches are independent; either can merge first. Both PRs touch
symbol_table.rsandapply_pending_context_changesin adjacent hunks,so whichever lands second expects a small manual rebase; the bench
helper duplicated between the two will be consolidated by whichever
lands second.
Test plan
cargo test --workspace --all-features: 15,003 tests pass.cargo clippy --workspace --all-features -- -D warningsandcargo fmt --check: clean.Caveat
All numbers above are aarch64. An x86_64 before/after run will be posted
on this PR before it's ready for merge.
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.