Repository navigation
Parsing optimizations - #331
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The perceptualHash reimplementation must be bit-for-bit identical to a third-party floating-point algorithm and affects persisted PhashDCT values, a correctness-sensitive property that cannot be fully verified without executing the compatibility test suite.
Review effort: Balanced
Findings: None
What changed in this PR
This PR introduces two behavior-preserving performance optimizations to the go-toolkit. First, it replaces the byte-at-a-time repeating-key XOR used for EPUB font deobfuscation with a new internal obfuscation package that expands the key with bulk copies and delegates to crypto/subtle.XORBytes. Second, it reimplements the DCT perceptual hash (previously github.com/azr/phash's DTC) as a local perceptualHash that computes only the 64 retained coefficients instead of all 1024, while deliberately preserving floating-point operation order so the output stays bit-for-bit identical to the reference. Both changes are backed by extensive compatibility tests and benchmarks.
Changes:
- Add
pkg/internal/obfuscation.XOR(repeating-key XOR via key expansion +subtle.XORBytes) and wire it intodeobfuscateFont. - Add
pkg/analyzer.perceptualHash, replacingphash.DTCinInspectImage, with the upstream MIT license preserved asLICENSE.phash. - Add thorough compatibility tests and benchmarks for both optimizations.
| File | Description |
|---|---|
| pkg/internal/obfuscation/xor.go | New repeating-key XOR using key expansion and subtle.XORBytes. |
| pkg/internal/obfuscation/xor_test.go | Correctness tests (many key/offset/size combos) and benchmark vs. modulo baseline. |
| pkg/parser/epub/deobfuscator.go | Replaces inline XOR loop with obfuscation.XOR; behavior preserved. |
| pkg/parser/epub/deobfuscator_test.go | Adds TestDeobfuscatorRanges exercising boundary spans for both algorithms. |
| pkg/analyzer/phash.go | New perceptualHash computing only 64 coefficients, preserving FP order for bit-compat. |
| pkg/analyzer/phash_test.go | Compatibility tests against phash.DTC across image types and near-median edge cases. |
| pkg/analyzer/image.go | Uses perceptualHash instead of phash.DTC for PhashDCT. |
| pkg/analyzer/image_bench_test.go | Benchmarks comparing reference vs. optimized hash across pipeline stages. |
| pkg/analyzer/LICENSE.phash | MIT license retained for the adapted phash code. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It reimplements security-relevant and hot-path logic (a hand-rolled streaming XML/namespace parser matching xmlquery semantics, font XOR, and perceptual hashing) whose many edge cases cannot be fully guaranteed by automated review despite strong test coverage, warranting human sign-off.
Review effort: Balanced
Findings: None
Asked Astra to check for parts of the go-toolkit that could take advantage of the new Go SIMD features. While that didn't really end up happening, it found some other minor optimizations.
Edit: I asked it to dive further into EPUB parsing optimizations specifically, and a lot of micro-optimizations (like allocating an array to the correct size once versus growing it in a loop in various places) but also some macro-optimizations like parsing the common layout of the OPF in a streaming manner and crawling the XML tree in various places instead of using queries. A lot of benchmarks were added. I also ran
go fixon the codebase addressing #294. Various results:OPF streamer
Across all 73 EPUBs, compared with the previous optimized build:
Various optimizations
Across 73 local EPUBs: