PivCoHuffman implementation - #61
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f80561440
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static T read(std::span<const std::byte> data, std::size_t& pos) { | ||
| T value; | ||
| std::memcpy(&value, data.data() + pos, sizeof(T)); | ||
| pos += sizeof(T); |
There was a problem hiding this comment.
When callers load a truncated or corrupt persisted stream through the public std::span<const std::byte> constructor, this helper unconditionally copies sizeof(T) bytes from data.data() + pos without checking that the bytes exist; a 1-byte span already reads past the buffer in the first header read, and later node/bitmap reads have the same issue. The deserializer should reject malformed streams before copying or indexing into decoded nodes rather than relying on the input being perfectly formed.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 90.10% 90.35% +0.25%
==========================================
Files 39 43 +4
Lines 8735 10010 +1275
Branches 1418 1612 +194
==========================================
+ Hits 7871 9045 +1174
- Misses 447 511 +64
- Partials 417 454 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex please run Huffman benchmarks and summarize throughput |
| @@ -0,0 +1,222 @@ | |||
| #include <benchmark/benchmark.h> | |||
There was a problem hiding this comment.
Please make it a general Huffman benchmark
| /// serialize the compressed stream. The compressed size is read from | ||
| /// the first timed iteration (no separate probe build), which keeps | ||
| /// gigabyte-scale setup cost to a single encode. | ||
| void BM_Encode(benchmark::State& state, DatasetMaker make) { |
There was a problem hiding this comment.
Please make it a general benchmark templated by the implementation
Huffman Benchmark SummaryI built the native benchmark targets and ran the complete set of 22 PivCo-Huffman encode/decode dataset variants at the 1 MiB input tier. This tier was selected to provide stable throughput measurements without conflating the results with the cache-resident 1 KiB cases or the substantial memory pressure of the 1 GiB cases. The benchmark’s supported sweep is 1 KiB, 1 MiB, and 1 GiB. src/benchmarks/pivco_huffman_benchmarks.cppL136-L142 The figures below are the median real-time throughput across five repetitions, using the benchmark’s default deterministic seed of 42. The harness explicitly uses real time, five repetitions, and aggregate-only reporting. src/benchmarks/pivco_huffman_benchmarks.cppL25-L34 src/benchmarks/pivco_huffman_benchmarks.cppL191-L198 Results
The Interpretation
The encode measurement includes Huffman-tree construction, bitmap population, and serialization. src/benchmarks/pivco_huffman_benchmarks.cppL146-L165 Decode measures reconstruction from an already-built in-memory codec; construction is outside the timed loop. src/benchmarks/pivco_huffman_benchmarks.cppL168-L183 Environment
Checks Performed
No repository files were changed, so no commit or follow-up pull request was created. |
|
Throughput comparison against upstream PivCo-Huffman Interoperability aside, the current implementation is materially slower overall, especially for AVX-512 encoding and fresh-stream decoding. I built an apples-to-apples harness around both public whole-stream APIs. It uses identical in-memory corpora, PH-only mode ( For decoding, the fair public-codec comparison is fresh-stream decode: construct Pixie from serialized bytes and call AVX-512 results16 MiB corpora, GiB/s, upstream production max-code-length-11 configuration:
Pixie warm-object decoding was still generally 1.15-1.7x slower. Rebuilding upstream with maximum code length 15 to match Pixie did not change the conclusion: Pixie lost every valid AVX-512 encode and decode workload. AVX2 resultsThe encoder result is more nuanced:
The AVX2 encoder has useful strengths for small alphabets and highly skewed data. The decoder lost every tested valid workload. Correctness blockerPixie fails to round-trip upstream bundled The current file benchmark does not validate decoded output, so it can report throughput for an incorrect result. This needs fixing before Pride or similar real-corpus throughput can be reported. Likely causes
Pixie output was also slightly larger on every valid workload, by approximately 0.0012-0.0315 bits per input byte, so the throughput loss is not buying a better compression ratio. Suggested priority
Based on these measurements, the current PR should not claim throughput parity with upstream from its existing benchmarks. |
2 participants
No description provided.