moribundfish-bit opened a new pull request, #2951:
URL: https://github.com/apache/iceberg-rust/pull/2951
## Which issue does this PR close?
No existing issue — the bug is described in full below.
## What changes are included in this PR?
`Record::serialize` in `crates/iceberg/src/spec/values/serde.rs` satisfies
serde's
`SerializeStruct::serialize_field(key: &'static str, ...)` bound with:
```rust
record.serialize_field(Box::leak(k.clone().into_boxed_str()), &v)?;
```
This permanently leaks a fresh clone of the field-name string for **every
field of
every record serialized** — memory growth of O(records × fields) that is
never
reclaimed for the lifetime of the process. Any long-running process that
commits
manifests continuously (e.g. a streaming writer using `fast_append`, or a
maintenance
job rewriting manifests) exhibits a monotonically increasing heap.
We hit this in production: jemalloc heap profiling attributed ~100% of a
steady
multi-hundred-MB/hour heap growth to this one call site, via
`write_manifest_file → apache_avro::to_value → RawLiteralEnum::serialize`.
An offline reproduction (in-memory catalog + local filesystem, no object
store)
driving repeated commits showed:
| files/commit | heap slope (stock) | heap slope (this fix) |
|---|---|---|
| 2000 | **+32 KB/commit, monotonic** | **flat (−36 B/commit)** |
| 50 | flat (leak below noise) | flat |
The leak scales with fields-per-commit, not snapshot count or table size.
`Box::leak` is present and identical in 0.8.0, 0.9.0, 0.10.0 and `main`.
Note: only `Record::serialize` is affected — `StringMap::serialize`
correctly uses
borrowed `&str` keys.
### The fix
Intern each **distinct** field name once in a process-lifetime table and
reuse the
`'static` reference thereafter:
```rust
record.serialize_field(intern_field_name(k), &v)?;
```
Total leaked memory drops from O(records × fields) to O(distinct field
names) — a
handful of schema field names, leaked once at warmup, reused forever.
Design notes:
- The `'static` requirement comes from serde's API, so *some* leak is
unavoidable
without changing the serialization shape (switching to `serialize_map`
would change
the Avro output from record to map). Interning is the minimal fix.
- The interned name is content-identical to the previously leaked copy, so
the
serialized bytes are unchanged. Validated by writing 16,000 entries with
the fix
and reading them all back through the unmodified deserialize path — all
entries
round-tripped intact.
- Lock contention is negligible: manifest serialization is sequential within
a
`write_manifest_file` call, and after warmup every lookup is an O(1) read
holding
the mutex for nanoseconds. The lock is poison-safe (`unwrap_or_else(|e|
e.into_inner())`).
## Are these changes tested?
Yes — new `interning_tests` module pins the two properties that together
guarantee
both the bounded leak and unchanged output:
1. **content equality** — interned name equals the original text (serialized
bytes
unchanged)
2. **pointer stability** — repeated names reuse a single allocation;
verified across
10,000 simulated records (bounded leak)
Plus: all existing `spec::values` tests pass (115 passed), `cargo fmt` and
`cargo clippy --all-targets -- -D warnings` are clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
