Workflow [PR], commit [66f976d]

Summary: ❌

AI Review

Summary

This PR changes the default x86 Linux target from x86-64-v2 to x86-64-v3 (AVX2) and removes related v2/v3 runtime-dispatch paths across the touched performance code. The live issue is in PR metadata: the changelog classification still does not match the backward-incompatible runtime requirement change.

PR Metadata

⚠️ Changelog category is currently Performance Improvement, but this PR changes the minimum CPU requirements for default amd64 binaries. That is a backward-incompatible user-facing change and should be categorized accordingly.

⚠️ Changelog entry still contains a TODO line (TODO: Change to Breaking Change after the CI is ok), which is not acceptable final changelog text.

Exact replacement:

### Changelog category (leave one):
- Backward Incompatible Change

### Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Default Linux `amd64` binaries now require `x86-64-v3` (`AVX2`+). Hosts without `AVX2` should use the `amd64compat` build; the installer auto-detects CPU capabilities and selects the compatibility artifact when needed.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions:

1. Change Changelog category to Backward Incompatible Change.
2. Remove the TODO and keep a final user-facing Changelog entry.

It shows both improvements and regressions in some cases. I'll keep it open to review it in depth and see if we can introduce dynamic dispatching where it benefits

Merged with master after a few of the improvements have been introduced via dynamic dispatch. I'll keep investigating other speed ups

Next to investigate is memcpy: We use our own, which is optimized for SSE4.2 and does 128 bit copies. AVX2 does 256 bit copies, and AVX512 would allow us to copy 512 bit per instruction. There was quite a bit of work done to introduce our own (#21520).

Some ideas:

• Test LLVM's libc implementation (https://github.com/ClickHouse/llvm-project/blob/7289b6789ef3297f8de30a9d94c7dabc6c35931e/libc/src/string/memory_utils/inline_memcpy.h)
• Test StringZilla sz_copy
• Test dynamic dispatch over our own implementation (we might need to tweak sizes and alignments)
• Re-evaluate glibc versions, but removing software prefetching
• Also test performance on ARM

Something that did not work: Prefetching

I tried different alternatives for memcpy but keeping our currently implementation seems the best option so far

Idk, if this could be useful for you or not. Afaik, the most conservative way to enable AVX2 would be to set -mprefer-vector-width=128 and +allow-light-256-bit (this article has more details). And yes, in the AVX2 context, it should be relevant only to older Intel chips, and if we're determined to enable, we should do so without crutches.

We currently use the default tuning for x86-64-v3 in clang which contains a bunch of tunings, including +allow-light-256-bit. But for example, when using dynamic dispatch for v4+ we had to add no-prefer-256-bit because the performance was noticeably worse in some cases, mostly because older Intel might need it but newer AMD became worse. It might be tricky to find one specific set up that works best for everything.

The cleanest approach is to assume a processor without throttling, therefore we won't need crutches (prefer-vector-width=128, allow-light-256-bit, no-prefer-256-bit etc.). ClickHouse Cloud runs on ARM anyways.

Most of the regressions should be covered now. The remaining red tests that I see are all within or at threshold. For example, bigint_arithm query 7 is +18.9% with threshold 18.9% and when testing locally v3 is slightly faster. I'll do another round of pure performance test before switching to check the whole CI.

Performance test noise analysis

The performance comparison reports show elevated thresholds and a high number of borderline regressions. This is expected and not indicative of real regressions.

Root cause of the noise: this PR compares a v2 (SSE4.2) baseline against a v3 (AVX2) build. Unlike normal PRs where both old and new binaries share the same ISA target and produce nearly identical machine code, here the two binaries have fundamentally different code generation — different instructions, different vectorization decisions, different instruction scheduling. The perf test alternates between old and new binary runs, and the different ISA targets leave different residual state in CPU caches, branch predictors, etc. This causes variable performance deltas between runs, leading to high variance and high thresholds.

Statistical summary (AMD shards, sha 8ba3244):

• 2090 queries tested
• 314 improvements above threshold vs 104 regressions above threshold (3:1 ratio)
• 87 of 104 regressions are within 2% of their threshold — consistent with the expected ~5% false positive rate
• Only 2 regressions significantly exceed their threshold, both on queries with very small absolute times (<12ms) where variance is inherently high

Local verification: all CI-reported regressions were benchmarked locally (master binary from CI vs local v3 build). None showed real regressions:

• not(toFloat32(number)) — reported at +40.4% in CI, locally identical (~0.17s on both)
• sum(toFloat32(number)) — v3 is faster after the vectorize(disable) fix
• sumKahan(toNullable(toFloat32(number))) — same speed
• kostikConsistentHash, number % 10 GROUP BY, intDiv(toInt256), decimal aggregates — all same speed or v3 faster

Note: the sum(toFloat32(number)) query [4] did not run in the latest CI execution, so the fix for that specific query cannot be confirmed from CI yet. A re-run should pick it up.

This noise will disappear once v3 becomes the baseline — future PRs will compare v3 vs v3, restoring normal threshold levels.

@rschu1ze @nickitat The PR is finally ready for review

Some notes:

• We use vzeroupper which, for calling small functions using AVX2, is bad for performance (more instructions, larger binary, less i-cache hits...). On the other hand, not using it on Intel would show huge degradations in performance because switching between AVX and SSE has large penalties. AMD would be fine, better in fact, without it. I opted out for the lesser evil

• There were some large regressions in some functions that I was unable to fix with directives or minor code changes. For now we fallback to compiling those files with v2 instead. In the future I'd like to remove them by doing larger refactors, or breaking the files to reduce its scope. I'd rather not do this in this PR, as there are plenty of "minor" changes already.

• Due to vzeroupper, code size increases, different code layout, etc., there are a bunch of functions where there might be a small degradation. I've addressed all that the performance tests have shown, focusing on anything that isn't flaky in master and showed 10+% change in CI. I've analyzed all of them on a Intel(R) Xeon(R) Platinum 8488C (EC2 C7i instances), matching what the CI runs.

Current performance report: https://pastila.nl/?00da47bb/5185028c8a271967d39877b08c189567#IQymHLWEeKoq+EhoEfv/iQ==GCM

Summary for 11e6269:

• Improvements: 52 queries (broad arithmetic / date-time / string / aggregation wins).
• Regressions: 7 flagged → 6 previously discarded as binary-layout/timing/master-flake noise → 1 (uniq_stored #1) fixed by 66f976d53eb.
• Variance: 50 unstable — mostly known master-flakes or shard wobble; no new actionable signal.
• *Net real regressions on AMD: 0.

We can keep iterating more, but at this point I think it's safe to review and merge.

LLVM Coverage Report

Changed lines: 95.28% (726/762) | lost baseline coverage: 25 line(s) · Uncovered code

Full report · Diff report

I've verified and checked locally the latest report and out of the regressions:

• agg_functions_min_max_any problem with loading data #79, join_used_flags #0, uniq_with_key_dist BaseDaemon: terminate on double SIGINT #3, bigint_formatting fix docs #6, replaceRegexp_fallback #0, trim_whitespace BaseDaemon: terminate on double SIGINT #3. All in shard 4. I tried to reproduce them locally but they didn't show any degradation.
• synthetic_hardware_benchmark fix docs #6 (shard 1). No degradation locally. In fact, it's ~10% faster

@Algunenano I actually reviewed an earlier state of the PR (also approved) and then the incremental commits you pushed since. Looks all good to me. Thanks for this PR!

Thanks a lot. Merging