Skip to content

SOW-0160: Go writer declares a 256 KiB XZ dictionary - #20

Open
ktsaou wants to merge 25 commits into
sow-0161/source-postingfrom
sow-0160/xz-dictionary
Open

ktsaou wants to merge 25 commits into
sow-0161/source-postingfrom
sow-0160/xz-dictionary

Conversation

@ktsaou

@ktsaou ktsaou commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

Stacked on #19. Completes SOW-0160 (.agents/sow/done/SOW-0160-20261015-go-xz-reader-dictionary-allocation.md).

Decision

  • User decision D1 = A (2026-11-03): the Go writer declares a 256 KiB XZ dictionary, the same value the Rust writer uses.

Why

  • Every XZ reader allocates the dictionary size the writer declares, once per DATA decode. The Go writer used the library default of 8 MiB, so a 600-byte payload cost an 8.4 MB allocation.
  • The reader can't lower this: ulikunitz/xz v0.5.16 uses the larger of the reader's cap and the stream's declared size, and it has no reset API for reusing a decoder.

Changes

  • go/journal/writer_compression.go: xzDictCap = 256 << 10 is now set in xzCompress.
  • New go/journal/writer_compression_xz_test.go:
    • A byte-level check that the declared dictionary is 256 KiB, with round trips of payloads up to 4 MiB (two of them larger than the dictionary).
    • A decode benchmark comparing 256 KiB, 1 MiB and 8 MiB declared dictionaries.
  • Spec (product-scope.md) and docs/Options-Reference.md record the declared dictionary.

Results

Go decode, 600 B payload 256 KiB 8 MiB
time 0.24-0.28 ms 1.6-2.0 ms
allocated 0.30 MB 8.42 MB

Test file: 5,000 Go-written XZ rows, exported with journalctl --file F -o export:

Reader 8 MiB file 256 KiB file
Rust journalctl 0.64-0.77 s user, 13 MB RSS 0.06 s user, 5 MB RSS
Go journalctl 22.5-27.2 s user 4.6-4.8 s user
stock journalctl v262 0.11-0.12 s 0.12 s (unaffected)
  • Exported output is byte-identical between the two files for every reader.
  • Files written before this change still declare 8 MiB and still read.

Validation

  • go test ./... and cargo test --workspace pass.
  • Compression matrix for xz, zstd and lz4: 72/72 pass.
  • Byte identity passes.
  • Go and Rust writers both declare 256 KiB (parsed from the matrix files).
  • SOW audit is clean.

Summary by cubic

The Go XZ writer now declares a 256 KiB LZMA2 dictionary instead of the library's 8 MiB default, matching the Rust writer. Reads of new Go-written XZ files cost the Go reader ~6x less time and ~28x less allocation per small decode, and the Rust journalctl runs ~10x faster; decoded content is unchanged and older 8 MiB files still read.

  • One constant in xzCompress sets the dictionary; no other writer path or dependency changes.
  • New tests parse the declared dictionary from the stream header, round-trip legacy 1 MiB and 8 MiB streams, and add a decode benchmark.
  • Seal tests in both SDKs skip when journalctl was built without sealing support, so negative seal tests cannot pass for the wrong reason.
  • Measured results refute the earlier inference that stock journalctl would benefit: liblzma decodes 8 MiB and 256 KiB files at the same cost.

Written for commit ee9c461. Summary will update on new commits.

View guided diff Turn on auto-fix

The Go reader's per-DATA XZ cost comes from the 8 MiB dictionary the Go
writer declares, not from reader configuration. Records measurements and
the writer dictionary-size options for a user decision.
@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: 4ab6d305-63f8-4525-af94-4fbd9cd02937

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

@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 33 complexity · 0 duplication

Metric Results
Complexity 33
Duplication 0

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 findings were identified. The analysis documents the allocation behavior and recommendation, but several acceptance areas remain unverified, including reproducible benchmarks, compatibility checks, and validation of the proposed writer configuration. No production-code or test changes are included, so these should be addressed before implementation or as part of the follow-up validation.

About this PR

  • The reported results still need a reproducible measurement harness, including separately retained-memory data and compatibility/round-trip validation before the documented recommendation is implemented.

Test suggestions

  • Reproduce XZ decode timing and allocation across the documented dictionary sizes.
  • Verify existing Go-written files remain readable without changing their declared dictionary size.
  • Validate the proposed 256 KiB writer configuration, including cross-language and large-payload round trips.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Reproduce XZ decode timing and allocation across the documented dictionary sizes.
2. Verify existing Go-written files remain readable without changing their declared dictionary size.
3. Validate the proposed 256 KiB writer configuration, including cross-language and large-payload round trips.

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 GoWriter as Go Writer
    participant RustWriter as Rust Writer
    participant Systemd as systemd journald
    participant XZLib as ulikunitz/xz v0.5.16
    participant GoReader as Go Reader
    participant LZMA2 as LZMA2 Filter Reader
    participant Alloc as Memory Allocator
    participant SOW as SOW Tracker

    Note over GoWriter,RustWriter: NEW: Documented dictionary size discrepancy

    GoWriter->>XZLib: xzCompress() - DictCap unset (default 8 MiB)
    XZLib->>XZLib: Uses library default 8 MiB dictionary
    XZLib-->>GoWriter: XZ stream with 8 MiB declared dict

    RustWriter->>XZLib: XzOptions::with_preset(0) - 256 KiB dict
    XZLib-->>RustWriter: XZ stream with 256 KiB declared dict

    Systemd->>XZLib: compress.c:180 - 1 MiB dict
    XZLib-->>Systemd: XZ stream with 1 MiB declared dict

    Note over GoReader,Alloc: NEW: Root cause - allocation follows stream declaration
    
    GoReader->>XZLib: Open XZ stream
    XZLib->>LZMA2: Initialize filter reader
    LZMA2->>LZMA2: Read stream header (declared dict size)
    alt Declared 8 MiB (Go writer)
        LZMA2->>Alloc: Allocate + zero 8.42 MB
        Alloc-->>LZMA2: Dictionary buffer
        Note over LZMA2: Decode time: ~1.5 ms
    else Declared 1 MiB (systemd)
        LZMA2->>Alloc: Allocate + zero 1.08 MB
        Alloc-->>LZMA2: Dictionary buffer
        Note over LZMA2: Decode time: ~0.45 ms
    else Declared 256 KiB (Rust writer)
        LZMA2->>Alloc: Allocate + zero 0.30 MB
        Alloc-->>LZMA2: Dictionary buffer
        Note over LZMA2: Decode time: ~0.17 ms
    end

    LZMA2-->>XZLib: Ready with fixed dictionary
    XZLib-->>GoReader: Per-DATA decompression

    Note over GoReader: NEW: Reader DictCap has no effect (max(reader cap, stream size))
    alt Reader attempts to set DictCap to minimum
        GoReader->>LZMA2: Set DictCap to 64 KiB
        LZMA2->>LZMA2: Still uses stream declared size
    end

    Note over GoReader: No decoder reuse available (no reset API)

    Note over SOW: NEW: Decision D1 recorded
    SOW->>SOW: Option A: 256 KiB (recommended, matches Rust)
    SOW->>SOW: Option B: 1 MiB (matches systemd)
    SOW->>SOW: Option C: keep 8 MiB
    Note over SOW: Existing files keep declared size under all options
Loading

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

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

Comment thread SOW-status.md Resolved
…emory; frame the gate as D1

- Compression loss of a 256 KiB dictionary is under 0.1% up to 1 MiB synthetic
  payloads and 0.75% at 4 MiB; allocation follows the declared dictionary.
- Retained memory returns to baseline after each decode; peak is the dictionary.
- Gate, plans and open decisions now describe the writer dictionary decision.
- Root index restores the not-activated / not-authorized guard.

@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 2 files (changes from recent commits).

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

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

@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 1 file (changes from recent commits).

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

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

@ktsaou ktsaou changed the title SOW-0160: XZ dictionary cost analysis (decision needed) SOW-0160: Go writer declares a 256 KiB XZ dictionary Nov 4, 2026

@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 6 files (changes from recent commits).

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

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

Stock verify seal tests in both SDKs now skip when journalctl was built without
forward-secure sealing, so negative tests cannot pass for the wrong reason.
A new test proves 1 MiB and 8 MiB XZ streams still decode. The SOW and ledger
report per-reader CPU figures and qualify the unmeasured stock-reader claim.

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