Skip to content

Remove redundant offset-array opens in the Rust writer - #17

Open
ktsaou wants to merge 14 commits into
sow-0123/fresh-realtime-seedfrom
sow-0137/array-open
Open

ktsaou wants to merge 14 commits into
sow-0123/fresh-realtime-seedfrom
sow-0137/array-open

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

Stacked on #16. Merge order: #13, #14, #15, #16, then this. Completes SOW-0137 (.agents/sow/done/SOW-0137-20260726-rust-writer-residual-array-open-performance.md).

Decisions for review (all agent-chosen; internal only, no byte or API change)

  • C1, taken: the global ENTRY-array append opens the tail array mutably once to read its capacity and write the slot, instead of a read-only open followed by a mutable open.
  • C2, taken: the regular DATA-array link keeps the tail capacity the chain walk already read, instead of reopening the tail.
  • C3, taken (after review): a new entry array gets slot 0 written at allocation, instead of being reopened. The first rejection was wrong: every new DATA object allocates an array, so mixed workloads hit this 34,157 times per 20,000 rows. Entry-array HMACs cover only the object header, so sealing is unchanged.
  • Not taken: regular-format DATA links still walk the array chain from the head. That is systemd's own regular-format algorithm, and removing it would need a new in-memory tail cache.

Mutable opens run the same type, size and bounds checks as read-only ones. Publication order is unchanged.

Measurement

Callgrind instructions at 20,000 rows, before → C1+C2 → C1+C2+C3:

Format Workload Before C1+C2 +C3
compact netflow 2119.1M 2114.3M (−0.2%) 2113.1M (−0.06%)
compact mixed 3060.1M 3054.8M (−0.2%) 3041.5M (−0.44%)
regular netflow 3980.7M 3777.0M (−5.1%) 3775.7M (−0.03%)
regular mixed 3940.6M 3788.4M (−3.9%) 3774.9M (−0.36%)

Release timing, baseline vs C1+C2+C3, regular format, 20 ABBA pairs per workload at host load 36-41: mixed +3.7% median (faster in 19/20 pairs), netflow +12.1% (19/20). The host was loaded, so timing is secondary evidence; an earlier -1.4% mixed run is not reproduced but remains unexplained.

Validation

  • Output is byte-identical to the baseline in all four configurations, with and without C3.
  • Stock journalctl --verify passes all four outputs.
  • cargo test --workspace passes.
  • Live matrix (go, rust and stock readers × regular, zstd, compact, sealed): 8/8.

Summary by cubic

Removes redundant offset-array opens in the Rust writer so regular-format writes execute 3.9-5.1% fewer instructions with byte-identical output. Completes SOW-0137.

  • The ENTRY-array append now opens the tail array mutably once instead of read-only then mutable.
  • The regular DATA-array link keeps the tail capacity the chain walk already read instead of reopening it.
  • Newly allocated entry arrays write slot 0 through the allocation guard, removing the reopen that followed each allocation.
  • Mutable opens run the same type, size, and bounds checks as read-only ones; publication order is unchanged.
  • Output is byte-identical in all four benchmark configurations, journalctl --verify passes, and the live reader matrix passes 8/8.

Written for commit 9dfa6e1. Summary will update on new commits.

View guided diff Turn on auto-fix

The global ENTRY-array append opened the tail array read-only for its
capacity and then mutably to write the slot; it now opens it mutably once.
The regular DATA-array link reopened the tail after the chain walk had
already read its capacity; it now keeps that value. Mutable opens run the
same type, size and bounds checks, and publication order is unchanged.

Regular-format writes execute 3.9-5.1% fewer instructions; output is
byte-identical. Completes SOW-0137.
@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9d4d6831-5e97-42a7-b4ad-9b736c693359

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@codacy-production

codacy-production Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics -13 complexity · -3 duplication

Metric Results
Complexity -13
Duplication -3

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

Run reviewer

TIP This summary will be updated as you push new changes.

@codacy-production codacy-production Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

✅ No blocking issues noticed

No blocking implementation findings were identified. The changed paths lack automated tests covering reuse, growth, validation, and reader interoperability; these should be added to protect the refactoring.

About this PR

  • Please add automated tests covering the changed ENTRY/DATA append paths, growth and linking behavior, malformed-input validation, and reader interoperability.

Test suggestions

  • Append an ENTRY entry when the tail has capacity, verifying the slot and header updates.
  • Append a regular DATA entry when the tail has capacity, using the capacity discovered during traversal.
  • Exercise ENTRY and DATA array growth paths and verify correct linking and tail metadata.
  • Reject malformed, wrong-type, undersized, or out-of-bounds arrays with the same validation behavior.
  • Verify byte-identical output and interoperability with journal readers.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Append an ENTRY entry when the tail has capacity, verifying the slot and header updates.
2. Append a regular DATA entry when the tail has capacity, using the capacity discovered during traversal.
3. Exercise ENTRY and DATA array growth paths and verify correct linking and tail metadata.
4. Reject malformed, wrong-type, undersized, or out-of-bounds arrays with the same validation behavior.
5. Verify byte-identical output and interoperability with journal readers.

TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Architecture diagram
sequenceDiagram
    participant W as Writer
    participant JF as JournalFile
    participant OA as OffsetArray

    Note over W,OA: ENTRY Array Append Flow (C1)
    W->>JF: entry_array_tail_entries()
    JF-->>W: tail_entries count
    W->>JF: offset_array_mut(tail_offset)
    JF->>OA: validate & open mutable
    OA-->>JF: capacity
    JF-->>W: tail_guard with capacity
    alt Room in tail array
        W->>OA: set(tail_entries, entry_offset)
        W->>JF: journal_header_mut()
        JF->>JF: update tail_entry_array_offset & n_entries
    else Tail array full
        W->>JF: drop(tail_guard)
        W->>JF: grow_entry_array_tail()
    end

    Note over W,OA: Regular DATA Array Link Flow (C2)
    W->>JF: append_data_entry_array_link()
    W->>JF: walk array chain from head
    JF->>OA: offset_array_ref (per array in chain)
    OA-->>JF: next_offset & capacity
    JF-->>W: tail_offset & tail_capacity (from walk)
    alt Entries fit in tail
        W->>JF: offset_array_mut(tail_offset)
        JF->>OA: validate & open mutable
        W->>OA: set slot with entry link
    else Tail full
        W->>JF: allocate & grow new array
        JF-->>W: new tail offset
        W->>JF: offset_array_mut(new_tail)
        W->>OA: set slot 0
    end

    Note over W,OA: Validation Remains (all paths)
    JF->>JF: type/size/arena checks before mutation
Loading

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Removes the reopen of each freshly allocated entry array to write slot 0.
Mixed-cardinality workloads allocate an array per new DATA object, so this
saves about 0.4% of writer instructions there; output bytes are unchanged.

This branch has not been deployed

No deployments
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