Skip to content

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
amazon-ion:mainfrom
hsbakshi:perf/large-doc-read
Draft

perf: reduce large-document binary Ion 1.0 read costs on the Element and AnyEncoding paths#1040
hsbakshi wants to merge 6 commits into
amazon-ion:mainfrom
hsbakshi:perf/large-doc-read

Conversation

@hsbakshi

Copy link
Copy Markdown

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 an
in-memory tree) and the lazy AnyEncoding path (the reader mode that
auto-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 FxHashMap
field-name index on Struct, and an inlining/variant-ordering pass over
the AnyEncoding dispatch layer. On value-heavy documents,
Element::read_one improves 10–13% on the criterion benchmark (7–8%
under an independent profiling harness; both aarch64), Struct::get by
name improves
30–37%, and the lazy AnyEncoding reader improves 9–12%. No public API
changes. 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:

  • On a ~30 KB value-heavy document, Element::read_one costs ~3.8x
    the concrete lazy reader (v1_0::Binary) reading the same bytes.
  • The AnyEncoding layer adds +48–49% over v1_0::Binary on
    value-heavy documents at both sizes. The gap grows with document size,
    so large documents do not amortize it away.
  • Building Struct's by-name field index shows ~12% of
    Element-path profile samples inside SipHash hashing frames (SipHash
    is the DoS-resistant hasher Rust's HashMap uses by default).
  • Local symbol table (LST) processing allocates per symbol with no
    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

  • b6cfb07 perf: add large-document read benchmark: new
    benches/large_doc_read.rs. Value-heavy and symbol-heavy documents at
    ~10 KB and ~30 KB; Element::read_one (default features); lazy
    AnyEncoding and v1_0::Binary readers (feature-gated like the
    existing benches); and a Struct::get by-name lookup flavor that
    includes ~13–17% miss lookups, so both hit and miss paths are guarded.
  • 6d6e61f perf: reserve symbol table capacity when applying a local
    symbol 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.
  • 4f0bcfd perf: use FxHashMap for Struct's by-name field index:
    swaps the index HashMap to FxHashMap (a much faster hasher that is
    not DoS-resistant). Rationale: field names are the same untrusted-input
    class as symbol text, and the existing ids_by_text index in
    symbol_table.rs already uses FxHashMap for that. The exposure
    profiles do differ: the symbol table precedent is per-stream and
    transient, while Struct is a retained DOM value; that difference is
    part of why the commit is severable. The hasher does not appear in any
    public signature. Struct's Debug output changes: the field-name
    index 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::Map
    convention), we're happy to drop this commit; it's severable.
  • 3a20f19 perf: inline AnyEncoding delegation and reorder hot
    dispatch enums
    : adds #[inline] to ~150 small AnyEncoding
    conversion/delegation functions, letting the compiler eliminate call
    overhead in this thin wrapper layer, and declares the Binary_1_0
    variants first in three internal-dispatch enums so the hot variant is
    matched first. Source-compatible: the three enums derive only
    Debug/Copy/Clone, with no Ord, no explicit repr, and no
    discriminant reads (verified). Debug::fmt and the expect_* error-path functions
    were 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).
  • ea09fc7 review polish: naming, comments, and bench cleanup from
    self-review.
  • bd3eb06 refactor: apply local review feedback: trims the
    #[inline] set to small dispatch/From shims and documents that
    convention 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 =
main plus the new benchmark commit; x86_64 run to follow before merge;
see caveat below). Values shown as 10 KB / 30 KB:

Case 10 KB 30 KB
element_read_one (value-heavy) −10.4% −13.5%
struct_get_by_name (incl. miss lookups) −30% −37%
lazy_any_read_all (value-heavy) −8.9% −11.7%
lazy_binary_1_0 (value-heavy) ±0 ±0
lazy_binary_1_0 (symbol-heavy) −6.4% −9.1%

The concrete v1_0::Binary value-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_structs and
encoding_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_one reproduced directionally at −7 to −8% under that
profiler'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 FxHashMap commit, per the hashing note
above) 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.rs and apply_pending_context_changes in 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 warnings and
    cargo fmt --check: clean.
  • No public API changes.

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.

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>)
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