Skip to content

perf: parse manifest list entries without buffering Avro values - #3224

Open
kacper-li-airspace-intelligence wants to merge 1 commit into
apache:mainfrom
kacper-li-airspace-intelligence:reduce-manifest-list-parser-memory
Open

kacper-li-airspace-intelligence wants to merge 1 commit into
apache:mainfrom
kacper-li-airspace-intelligence:reduce-manifest-list-parser-memory

Conversation

@kacper-li-airspace-intelligence

Copy link
Copy Markdown

What changes are included in this PR?

ManifestList::parse_with_version collects the entire Avro reader into a Value::Array before deserializing it. Each manifest stays in memory as a Value::Record, with owned field names, strings and nested values. from_value then borrows that array while allocating the typed records, so both representations coexist.

This becomes expensive when a snapshot contains many manifests. For example, FastAppendAction::existing_manifest loads the current manifest list on each append, even if the new append is small. The temporary allocation cost grows with the existing list, not the size of the append.

Convert each Avro record into its final ManifestFile as it is read, then drop the generic value before reading the next record. V1 keeps its reader-schema projection; V2/V3 keep the writer schema, field aliases and defaults. The input bytes and final typed list still occupy memory. Parsing still returns an error without a partial list, although a conversion error can now precede a later Avro decoding error.

Remove the unused whole-list conversion methods and keep the serialization wrappers used by the existing tests behind cfg(test).

Are these changes tested?

The added manifest_list_memory test generates 10,000 distinct manifest paths with one partition summary per entry. It checks every parsed entry and measures peak live allocations with a test-only wrapper around System. On aarch64 macOS, Rust 1.96.0, release mode:

Uncompressed V2 list: 692,289 input bytes Peak additional live allocations
Main at 9db0b685 32,874,672 bytes
This change 6,040,920 bytes

The 16 MiB regression budget fails on main and passes with the patch. Input generation is excluded from the measurement. Snappy and Zstandard cases also pass; the test additionally checks V1/V2/V3 round trips, empty lists, V1 defaults when read as V2, and rejection of truncated input.

cargo +stable test -p iceberg --release --test manifest_list_memory --locked -- --nocapture

To reproduce the baseline, keep the added test and restore crates/iceberg/src/spec/manifest_list/{mod.rs,_serde.rs} from 9db0b685, then run the same command.

All 1,691 core unit tests pass, including the existing schema-projection, field-alias and encrypted manifest-list tests. Clippy and formatting pass with the repository's pinned nightly toolchain.

AI Disclosure

AI assisted with the implementation, test and description.

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