[improvement](build) Upgrade Snappy to 1.2.1 and enable SIMD paths - #67727
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: StarRocks/starrocks#78566 Problem Summary: Doris builds Snappy 1.1.10 without explicitly enabling its SIMD paths. Upgrade to 1.2.1, enable SSSE3 on x86_64 within the BE's existing SSE4.2 baseline, and honor ARM_MARCH (default armv8-a+crc) on ARM. Retain RTTI for SnappySlicesSource and remove the sign-compare patch included upstream. Reuse the existing codec and test its binary data, block boundaries, empty slices, malformed streams, output capacity, and frozen 1.1.10 data. On a Xeon Platinum 8457C, a seven-run median CPU-time microbenchmark of 1 MiB repeated binary data improved compression by 24.2% and decompression by 126.7% against 1.1.10 with the prior build flags, using Clang 16.0.6 for both builds. Results vary by input: HTML compression regressed by 12.4%. These are hot-buffer library measurements on a shared host, not SQL or ARM performance results. The upstream larger hash table adds up to 32 KiB of temporary compression memory. ### Release note Upgrade the BE Snappy dependency to 1.2.1 and enable SIMD paths within the existing supported CPU baseline. Snappy data remains format-compatible; performance and compressed bytes can vary by input. ### Check List (For Author) - Test: Unit Test / Manual test - Run run-be-ut.sh -j 48 --run --filter=BlockCompressionTest.* with ASAN: all 5 tests pass. - Build Snappy with thirdparty/build-thirdparty.sh -j 48 snappy. - Verify both directions of 1.1.10/1.2.1 decoding on 24 inputs each. - Run CPU-time compression/decompression microbenchmarks on 12 inputs. - Verify AArch64 CRC32 intrinsic code generation (no ARM hardware test). - Pass shell syntax, build hygiene, clang-format 16, and changed-line clang-tidy checks. - Behavior changed: Yes. Compression output and performance may change; the data format, codec interfaces, and default compression selection remain compatible. - Does this need documentation: No.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
### What problem does this PR solve? Problem Summary: The Snappy recipe only enabled SSSE3 and ignored the BE's USE_AVX2 setting. Build with the existing SSE4.2 baseline when AVX2 is disabled, and add -mavx2 by default or when enabled. This enables CRC32 hashing and, for AVX2 builds, vector copy paths without implicitly requiring BMI2. Document that third-party libraries must be rebuilt with the matching target. ### Release note Snappy third-party builds now honor USE_AVX2. Use USE_AVX2=0 or OFF for the SSE4.2 target; the default target enables AVX2. Performance varies by input. ### Check List (For Author) - Test: Manual test: build both targets with build-thirdparty.sh -j 48 snappy; inspect generated instructions; verify 24 inputs across all 9 combinations of old/SSE4.2/AVX2 encoders and decoders; run seven-trial microbenchmarks; check 30 architecture/flag combinations, Bash syntax, recipe formatting, and no new ShellCheck warnings. BE unit tests were not rerun for this update. - Behavior changed: Yes. Build flags and compression output/performance can change; the Snappy wire format remains compatible. - Does this need documentation: No. Build behavior is recorded in the changelog.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two blocking ISA-selection defects remain in the Snappy build integration.
Review checkpoints:
- Goal and proof: the dependency/version update and codec tests are focused and the new binary, boundary, malformed-input, capacity, and frozen-1.1.10 oracles are sound, but they do not prove that the final BE links a Snappy archive matching its selected CPU target.
- Lifecycle and configuration: the static archive has an independent reusable lifetime with no ISA fingerprint or rebuild edge, and the shell parser does not share CMake's boolean semantics. These are the two inline findings.
- Compatibility and parallel paths: the checksum, source directory, obsolete-patch removal, RTTI, public ABI, raw format, install layout, direct BE import, Arrow SYSTEM selection, and native Linux/macOS branches are otherwise consistent. Hadoop framing and bRPC's separate Snappy path are unchanged.
- Concurrency and ownership: no new shared mutable state, lock ordering, static-initialization, or ownership defect was found in the synchronous codec path.
- Performance and error handling: the disclosed working-memory increase and input-dependent compression regressions were considered; malformed/capacity failures propagate correctly and no additional distinct issue survived.
- Persistence, transactions, FE/BE transport, nullable handling, runtime-dynamic configuration, and observability are not applicable to this build/dependency change.
Validation was review-only as required by the runner: authoritative diff and whole-path inspection, shell syntax, CMake truth-table/compiler-preprocessor checks, checksum/upstream source verification, and the repository build-hygiene check. I did not build BE or run BE unit tests; the PR description also says those tests were not rerun after the final ISA recipe revision.
| fi | ||
|
|
||
| local snappy_cxx_flags="-O3" | ||
| case "$(uname -m)" in |
There was a problem hiding this comment.
[P1] Tie this ISA-specific archive to the BE target
The installed libsnappy.a is now target-specific, but its lifetime is not tied to the BE target. The published x86/ARM build images run this script with the defaults, while build.sh reuses any complete third-party tree solely by checking the last unrelated library. Consequently build-for-release.sh --noavx2 can compile BE without AVX2 but link this default AVX2 archive; similarly, ARM_MARCH=armv8-a can reuse the default +crc archive. Upstream Snappy 1.2.1 has reachable unconditional _mm256_* operations under __AVX__ and emits ARM CRC instructions when built with +crc. The BE startup check cannot catch the mismatch because its AVX probes are compiled from the BE's own target macros, and the ARM path does not probe CRC. These nominal lower-target packages can therefore SIGILL on the CPUs they are intended to support. Please keep the reusable archive at the baseline, add target-specific variants/runtime dispatch, or fingerprint it and force selection/rebuild whenever the BE target changes.
| x86_64) | ||
| # Match the BE's SSE4.2 baseline and optional AVX2 target. | ||
| snappy_cxx_flags+=" -msse4.2" | ||
| case "${USE_AVX2:-ON}" in |
There was a problem hiding this comment.
[P2] Normalize USE_AVX2 before branching
This case does not use the same truth table as the BE's later if(USE_AVX2) in CMake. CMake treats named boolean constants case-insensitively and also treats N, IGNORE, NOTFOUND, and *-NOTFOUND as false, but values such as USE_AVX2=Off fall through here and add -mavx2 while BE omits it. That creates an AVX2 Snappy archive inside an otherwise non-AVX2 fresh build and can fault at runtime. Please normalize or reject the setting once, or implement the exact CMake false set, so both build stages select the same target.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
3 participants
What problem does this PR solve?
Doris builds Snappy 1.1.10 without enabling its available SIMD paths. Upgrade to 1.2.1 and match the BE instruction-set target:
-mavx2by default and whenUSE_AVX2is enabled;USE_AVX2=0orOFFselects the SSE4.2 baseline. This follows the existing third-partyUSE_AVX2convention used by CRoaring.ARM_MARCH(defaultarmv8-a+crc).Snappy is a prebuilt static library, so its third-party build must use the same target as the BE. Changing only the BE's
USE_AVX2setting does not rebuild or retarget an existing Snappy archive. We do not enableSNAPPY_REQUIRE_AVX2, because upstream 1.2.1 adds BMI2 through that option on GCC/Clang, while BMI2 is not guaranteed by AVX2.Preserve RTTI for
SnappySlicesSourceand remove the sign-compare patch already included upstream. Existing codec interfaces and default compression selection remain unchanged; bRPC's embeddedbutil::snappyis outside this change.Add coverage for binary data, 64 KiB boundaries, empty and uneven slices, truncated and invalid streams, insufficient output capacity, and frozen 1.1.10 compressed data. Use byte-exact comparisons in the existing round-trip tests.
Fresh seven-run median CPU-time microbenchmarks on a Xeon Platinum 8457C, using Clang 16.0.6 for all libraries, gave the following throughput changes relative to 1.1.10. Both new targets were built through the updated third-party script. These results replace the measurements for the earlier SSSE3-only recipe.
Compression regresses on some inputs; this is not a universal speedup. Measurements are single-threaded, pinned to one logical CPU on a shared host, with hot input, preallocated output buffers, and at least 0.1 CPU seconds per sample. Twelve inputs were measured. These are library microbenchmarks, not SQL or ARM performance claims. The larger upstream hash table adds up to 32 KiB of temporary compression memory.
Release note
Upgrade the BE Snappy dependency to 1.2.1 and enable SIMD paths for the selected CPU target. Third-party Snappy builds enable AVX2 by default; use
USE_AVX2=0orOFFfor a non-AVX2 BE. Snappy data remains format-compatible; performance and compressed bytes can vary by input.Check List (For Author)
USE_AVX2=OFF/ON thirdparty/build-thirdparty.sh -j 48 snappywith Clang 16.0.6.git diff --checkpass. ShellCheck introduces no new warnings; 7 existing warnings remain elsewhere in the script.run-be-ut.sh -j 48 --run --filter='BlockCompressionTest.*'passed all 5 tests under ASAN, with build hygiene, clang-format 16 and changed-line clang-tidy passing. These BE tests ran before the Apache-master cherry-pick and before the SSE4.2/AVX2 flag update; they were not rerun for the final targets. CI must validate final BE integration.