Repository navigation
Optimize scalar LZ4 compression and decompression on C906 - #505
dormancygrace wants to merge 1 commit into
Conversation
Build outputsCompared with the build of main at c93fb1a; codex/lz4-zram-table4k is at 2ed1db1, so outputs changed since then are listed too. 17 changed, 0 added, 0 removed of 570 outputs.
|
dormancygrace
left a comment
dormancygrace
left a comment
There was a problem hiding this comment.
The new code is correct.
- Differential: I compared the old and new decoders on 309,540 constructed cases (offsets 0–16, match lengths 4–70, 0–20 trailing literals, with the safe, fast and partial APIs) and on 300,000 bit-flipped or truncated streams under ASan. The results were identical.
- bswap+clz: it is only reached with a non-zero
diff, and the kernel build always enables XTheadBb.
My main question is about removing 0039.
| @@ -336,7 +336,6 @@ CONFIG_TOOLCHAIN_HAS_V=y | |||
| CONFIG_RISCV_ISA_V=y | |||
| CONFIG_RISCV_ISA_V_DEFAULT_ENABLE=y | |||
| CONFIG_RISCV_ISA_V_UCOPY_THRESHOLD=768 | |||
There was a problem hiding this comment.
This removes 0039 and CONFIG_RISCV_C906_LZ4_DECOMPRESS. firmware/alpine/bench/c906-lz4/README.md documents that patch as the selected production change: paired decode time −17.9% and real-zram read time −17.8% on C906. The kernel is built with -mstrict-align (there is no RISCV_EFFICIENT_UNALIGNED_ACCESS), so an 8-byte LZ4_memcpy of unknown alignment is byte-wise, and 0039's aligned path may have been the real win.
The −1.93% decoder figure here does not say whether its baseline had 0039 enabled. Please state the baseline (current main with the option on) and repeat the real-zram read comparison for this branch before merging. Please also update or remove the bench README's "Selected production change" section and selected.config, which still point at 0039.
|
|
||
| if (unlikely(offset < 8)) { | ||
| + /* Keep offset-zero matches initialized. */ | ||
| + if (!partialDecoding) |
There was a problem hiding this comment.
This only holds for full decodes. With LZ4_decompress_safe_partial the write is still skipped, as before, so an offset-0 match in partial mode still leaves stale bytes. That is not a regression. Please narrow the comment, or write the 4 bytes in partial mode as well when oend - op >= 4.
| int pos = prefix + i, from = pos - offset; | ||
| expected[pos] = offset == 0 ? 0 : from < 0 ? dictionary[4096 + from] : expected[from]; | ||
| } | ||
| input[p++] = 12 << 4; |
There was a problem hiding this comment.
Every constructed block ends with exactly 12 literals. So for offsets below 8, where the new 4-byte write lives, the near-end path (cpy > oend - MATCH_SAFEGUARD_DISTANCE) is never reached.
Please add tails of 5–11 literals. Please also add an out-of-range offset and an overflowing length extension, so that "malformed input" in the description is covered by more than truncation.
C906 spends avoidable instructions in LZ4 byte counting and match copies. Use byte reversal plus CLZ in the compressor, initialize the decoder overlap buffer only for offsets below eight, and remove the old aligned-copy dispatch and its kernel option. Public APIs, bounds checks and compressed output are preserved.
Measured with the actual kernel code on C906, using 1,413 identical 4 KiB pages and matched code placement:
These are codec measurements, not end-to-end zram gains. Decoder inputs were produced by the exact compressor in #503. Each comparison used bracketing baselines, three rounds and two layouts; decoder candidate order was rotated. All device runs passed, with unchanged output.
Validation: normal and ASan/UBSan compressor, small-table and decoder contracts; guarded buffers and malformed input; overlap distances, partial decoding, dictionaries, safe/fast streaming and in-place decompression. A negative control confirms that the decoder test detects missing zero-offset initialization. Final C906 machine code matches the measured prototypes; patch application, platform tests and kernel configuration checks pass. The decoder contract now runs against the actual patched kernel source in firmware CI.
Stacked on #503; compressor update #501 and table-size change #503 remain separate.