You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "moribundfish-bit (via GitHub)" <gi...@apache.org> on 2026/08/03 17:52:15 UTC
[PR] fix(spec): avoid unbounded memory growth in Record::serialize by interning field names [iceberg-rust]
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(spec): avoid unbounded memory growth in Record::serialize by interning field names [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2951:
URL: https://github.com/apache/iceberg-rust/pull/2951#discussion_r3932211385
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -73,10 +97,10 @@ pub(crate) mod _serde {
let len = self.required.len() + self.optional.len();
let mut record = serializer.serialize_struct("", len)?;
for (k, v) in &self.required {
- record.serialize_field(Box::leak(k.clone().into_boxed_str()), &v)?;
+ record.serialize_field(intern_field_name(k), &v)?;
Review Comment:
I don't think this is the right direction. Instead of serializing as struct, we could serialize as a map and we could then avoid interning.
##########
crates/iceberg/src/spec/values/serde.rs:
##########
@@ -716,4 +740,61 @@ pub(crate) mod _serde {
}
}
}
+
+ #[cfg(test)]
Review Comment:
Put the tests in values's tests mod
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(spec): avoid unbounded memory growth in Record::serialize by interning field names [iceberg-rust]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2951:
URL: https://github.com/apache/iceberg-rust/pull/2951#issuecomment-5518420355
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org