perf(journald): avoid cloning the MESSAGE payload per entry#3403
perf(journald): avoid cloning the MESSAGE payload per entry#3403timr-dev wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the journald receiver hot path by removing a per-entry duplicate allocation/copy of the MESSAGE payload. Instead of storing an owned message_body buffer alongside fields, it stores message_body_index: Option<usize> (pointing into fields) and reads the body via a JournalEntry::message_body() accessor, preserving the emitted OTLP output contract.
Changes:
- Replace
JournalEntry.message_body: Option<Vec<u8>>withmessage_body_index: Option<usize>in the journald read/decode path. - Update the Arrow records encoder to read the body via
JournalEntry::message_body()(borrowing fromfields). - Add a Rust
.chloggenentry documenting the performance improvement.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rust/otap-dataflow/crates/core-nodes/src/receivers/journald_receiver/journal.rs | Stores the chosen MESSAGE field position as an index instead of cloning the payload. |
| rust/otap-dataflow/crates/core-nodes/src/receivers/journald_receiver/arrow_records_encoder.rs | Adds message_body() accessor and switches body projection to borrow bytes from fields; updates tests to use the index. |
| rust/otap-dataflow/.chloggen/journald-message-no-double-copy.yaml | Release note entry for the journald receiver allocation/copy reduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3403 +/- ##
========================================
Coverage 86.23% 86.24%
========================================
Files 739 740 +1
Lines 292341 292548 +207
========================================
+ Hits 252107 252312 +205
- Misses 39710 39712 +2
Partials 524 524
🚀 New features to boost your workflow:
|
1264a55 to
dc4ba1b
Compare
lalitb
left a comment
There was a problem hiding this comment.
LGTM. Thanks for optimizing this. Have couple of nit comments, and should be good to merge then.
`current_entry` cloned the `MESSAGE` field value into `JournalEntry.message_body` while the same bytes were also pushed into `fields`, so the message payload (the largest field in a typical entry) was allocated and copied twice per record. Store `message_body_index: Option<usize>` -- the index of the chosen `MESSAGE` field in `fields` -- instead of a second owned copy, and read the body back through `JournalEntry::message_body()`. This removes one `Vec<u8>` allocation and memcpy of the message payload per entry that has a MESSAGE, with no new allocations (the index is a `usize`). Testability & coverage ---------------------- The per-field decode loop lived inside the Linux-only `imp::current_entry` FFI method, where it could not be unit-tested (no live journal) and left the changed lines uncovered (failing `codecov/patch`). It is extracted into a pure, cross-platform `decode` module as a stateful `FieldDecoder` (`new`/`feed`/ `finish`) with 13 unit tests covering the drop, truncation, MESSAGE-selection, duplicate-MESSAGE, non-UTF8, cap, and empty-message paths. `current_entry` is now a thin FFI adapter over it. Sound FFI field iteration ------------------------- `sd_journal_enumerate_data` returns a pointer that is only valid until the *next* enumerate call. The reader now exposes it through a *lending* `EnumerateData::next_field(&mut self) -> Option<&[u8]>`: the returned slice borrows `&mut self`, so the borrow checker forbids fetching (or aliasing) another field while it is held -- modelling the FFI's lifetime exactly. It is deliberately not a `std::iter::Iterator`, whose non-lending `Item` would let safe code `collect()` the slices into a use-after-free. Benchmark (decode-only) ----------------------- `benches/journald_decode` (a criterion bench behind the `bench` feature) isolates the field-decode step only -- it is NOT an end-to-end receiver benchmark (after decode the receiver still copies each kept field into the Arrow builders). A one-time local comparison (Linux, LTO off) of the decode with vs without the removed MESSAGE clone, per entry: MESSAGE size | decode (index body) | decode + body clone (pre-PR) | delta -------------+---------------------+------------------------------+---------------- 256 B | ~0.70us | ~0.90us | clone +28% 4 KiB | ~0.94us | ~1.00us | clone +7% 64 KiB | ~2.53us | ~4.08us | clone +61% (-1.55us) The saving is the eliminated per-entry allocation + memcpy of the MESSAGE payload; decode-side it is near the noise floor for tiny messages and dominant for large ones (~38% faster decode at 64 KiB). Per reviewer feedback the live bench now times only the current decode; the comparison above is preserved as a documented reference result in the bench's module doc. `message_body()` carries a `debug_assert` that the indexed field is the MESSAGE field, to catch a future refactor that reorders or drops fields between decode and encode. Contract preservation --------------------- The emitted OTLP output is byte-for-byte unchanged: the body is still the value of the first non-dropped `MESSAGE`, and `MESSAGE` is still emitted as an attribute. `FieldDecoder` reproduces the old loop exactly -- first-MESSAGE detection before the drop check (so a dropped first `MESSAGE` permanently leaves the body unset even if a later `MESSAGE` survives), identical drop ordering, non-UTF8-name drop, and no-`=` skip. The pinned encoder tests -- including that dropped-first-MESSAGE edge case -- pass unchanged. Tests: 13 new `decode` unit tests + 1 bench-helper smoke test; existing encoder / config / fresh-start / checkpoint / imp tests unchanged (61 journald tests green on Linux; clippy `-D warnings` clean incl. `--features bench`). Files: crates/core-nodes/src/receivers/journald_receiver/decode.rs (new) crates/core-nodes/src/receivers/journald_receiver/journal.rs crates/core-nodes/src/receivers/journald_receiver/mod.rs crates/core-nodes/src/receivers/journald_receiver/arrow_records_encoder.rs crates/core-nodes/benches/journald_decode/main.rs (new) crates/core-nodes/Cargo.toml Addresses the MESSAGE double-copy in open-telemetry#3398; per-entry buffer reuse remains a possible follow-up under that issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
dc4ba1b to
be952ef
Compare
Summary
Addresses the
MESSAGEdouble-copy in #3398.SdJournalReader::current_entry()cloned each entry'sMESSAGEfield value intoJournalEntry.message_bodywhile the same bytes were also pushed intofields,so the message payload — usually the largest field in an entry — was allocated
and copied twice per record.
This stores
message_body_index: Option<usize>(the index of the chosenMESSAGEfield infields) instead of a second owned copy, and reads the bodyback through a new
JournalEntry::message_body()accessor. OneVec<u8>allocation + memcpy of the message payload is removed per entry that has a
MESSAGE; no new allocations are introduced (the index is ausize).Output is unchanged (contract preserved)
append()still emits the log body from the selectedMESSAGEand still emitsMESSAGEas an attribute. The body source is the first non-droppedMESSAGEchosen at decode time: when the first
MESSAGEis dropped for size and a laterone survives, the body must stay unset. Storing the exact chosen index preserves
this. The pinned encoder tests, including that edge case
(
leaves_body_unset_when_first_message_was_dropped), pass unchanged.Testable decode + patch coverage
The per-field decode loop lived inside the Linux-only
imp::current_entryFFImethod, so its changed lines could not be unit-tested (no live journal) and were
left uncovered (failing
codecov/patch). It is extracted into a pure,cross-platform
decodemodule as a statefulFieldDecoder(new/feed/finish), andcurrent_entryis now a thin FFI adapter over it. 13 new unittests cover the drop, truncation,
MESSAGE-selection, duplicate-MESSAGE,non-UTF-8, field/entry-cap and empty-message paths — moving the previously
uncovered logic into covered code.
Sound FFI field iteration
sd_journal_enumerate_datareturns a pointer valid only until the nextenumerate call. The reader exposes it through a lending
EnumerateData::next_field(&mut self) -> Option<&[u8]>: the returned sliceborrows
&mut self, so the borrow checker forbids fetching (or aliasing) anotherfield while it is held — modelling the FFI's lifetime exactly. It is deliberately
not a
std::iter::Iterator, whose non-lendingItemwould let safe codecollect()the slices into a use-after-free.Benchmark (decode-only)
benches/journald_decode(criterion, behind thebenchfeature —cargo bench -p otap-df-core-nodes --features bench) isolates the field-decodestep only — it is not an end-to-end receiver benchmark (after decode the
receiver still copies each kept field into the Arrow builders). A one-time local
comparison (Linux, LTO off) of the decode with vs without the removed
MESSAGEclone, per entry:
MESSAGEsizeThe saving is the eliminated per-entry allocation + memcpy of the
MESSAGEpayload; decode-side it is near the noise floor for tiny messages and
dominant for large ones (~38 % faster decode at 64 KiB). Per reviewer feedback
the live benchmark now times only the current decode; the comparison above is
preserved as a documented reference result in the bench's module doc.
Testing
cargo test -p otap-df-core-nodes— 13 newdecodeunit tests + the existingencoder output-contract tests (body + attribute projection, dropped-first-
MESSAGEedge case) pass, confirming byte-identical OTLP output.cargo clippy --lib --tests --benches --features bench -- -D warningsclean; the full journald suite (61 tests, incl. the Linux-only
impFFI tests)passes.
cargo buildclean (no dead-code).cargo fmtclean.Per-entry buffer reuse (
cursor/ fieldname/value) — the secondary itemin #3398 — is deliberately left out; it reworks
JournalEntry/JournalFieldownership more broadly and warrants its own PR, so #3398 stays open to track it.