build!: optimize for x86-64-v3 cpu microarchitecture (Haswell+) #2374
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces configuration updates for Rust compilation targeting x86_64 architectures across multiple files. The changes focus on enhancing build configurations in Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lklimek
changed the title
|
According ChatGPT the microarchitecture level x86-64-v3 will actually exclude Haswell CPU's and apply to Skylake or higher :
Update : A new chat session with ChatGPT shows this Haswell CPU (and Haswell CPU's in general) after a misleading chat start does conform to x86-64-v3 :
To check in Ubuntu : lscpu | grep -o -E 'avx|avx2|bmi1|bmi2|fma|movbe|lzcnt|popcnt' | sort | uniq -c
So x86-64-v3 does seem to support my cpu after all and GLIBC-wise there is no problem either (i am on GLIBC 2.35). But maybe best to let me test this pull request merged in a future dev build on my VPS ? Just to be sure it will actually work on Haswell CPU's. Update : my VPS provider also confirmed to me that my CPU conforms to X86-64-v3 and added this little piece of information :
So its not just the flags that need to be in there and the Haswell microarchitecture of the CPU, the qemu version seems to play a role as well (needs to be greater than 7.2) |
|
@coderabbitai review all |
|
@lklimek, I will review all the changes in the PR as per your request. ✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
280-283: Remove duplicateONBUILD ARG CARGO_BUILD_PROFILEdeclarationThe
ONBUILD ARG CARGO_BUILD_PROFILE=devis declared twice (lines 117-118 and 282-283). This duplication is unnecessary and can be removed to simplify the Dockerfile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cargo/config-release.toml(1 hunks).cargo/config.toml(1 hunks)Dockerfile(3 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-abci/Cargo.toml (1)
Learnt from: shumkov
PR: dashpay/platform#2375
File: packages/rs-drive-abci/Cargo.toml:61-63
Timestamp: 2024-12-05T09:29:38.918Z
Learning: In the `drive-abci` package, avoid adding unused dependencies like `hashbrown` to `Cargo.toml`. The team relies on CI to detect dependency version issues.
🔇 Additional comments (5)
Dockerfile (1)
125-138: Verify CPU compatibility for optimization flags
The compiler flags -march=x86-64-v3 and -march=x86-64 set in the environment variables CFLAGS and CXXFLAGS optimize the build for specific CPU architectures. Ensure that all target deployment environments support these architectures to prevent runtime errors on unsupported CPUs.
Run the following script to list all occurrences where these flags are used and verify compatibility:
✅ Verification successful
Let me gather more context about the build configuration and architecture-specific settings.
Based on the gathered context, I can now provide a final response:
CPU optimization flags are properly configured with architecture safeguards
The optimization flags are safely implemented with proper architecture checks:
- For AMD64 release builds: Uses
-march=x86-64-v3for modern CPU optimizations - For AMD64 debug builds: Uses conservative
-march=x86-64baseline - For non-AMD64 architectures (e.g., ARM64): No architecture-specific flags are set (
PORTABLE=1)
The Dockerfile includes proper architecture detection and fallbacks, making it safe across different platforms. The build system correctly handles both AMD64 and ARM64 architectures with appropriate conditional logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all Dockerfiles and check for usage of -march flags
fd Dockerfile | xargs grep -n '\-march='
Length of output: 292
Script:
#!/bin/bash
# Check for any architecture-specific conditions and build profiles
rg "TARGETARCH|CARGO_BUILD_PROFILE" -A 3 -B 3
# Check for any documentation or comments about CPU requirements
fd "README|INSTALL|CONTRIBUTING" -e md | xargs rg -i "cpu|processor|architecture|requirement"
Length of output: 6330
.cargo/config-release.toml (1)
8-11: Ensure consistency of rustflags across targets
The addition of -C target-cpu=x86-64-v3 to the rustflags for both x86_64-unknown-linux-musl and x86_64-unknown-linux-gnu targets is appropriate for optimizing performance on newer CPUs (Intel Haswell and newer). Ensure that this aligns with the project's hardware compatibility requirements.
.cargo/config.toml (2)
7-15: Update rustflags for x86_64-unknown-linux-musl target
The addition of -C target-cpu=x86-64 in the rustflags enhances performance while maintaining compatibility with standard x86-64 architectures in debug builds.
16-24: Add configuration for x86_64-unknown-linux-gnu target
Including rustflags for the x86_64-unknown-linux-gnu target ensures consistent build settings across similar architectures.
packages/rs-drive-abci/Cargo.toml (1)
117-118: Review the addition of the [lints.rust] section
The new [lints.rust] configuration helps in catching unintended usage of cfg(tokio_unstable) by issuing warnings for unexpected conditional compilation flags. This can improve code quality by alerting developers to potential misuse.
Note: Based on previous learnings from shumkov, the team prefers to minimize additions to Cargo.toml unless necessary. Since this configuration enhances code safety without introducing new dependencies, it aligns well with project practices.
@qwizzie I've updated the PR description with info on how to test these changes yourself. Any feedback appreciated :). |
That works fine, thanks. It passes (0) on my VPS, but does not show version :
It also shows 0 with the debug test and passes without any error. But i am not seeing any version in the debug test.
|
It's fine, my mistake (I changed the script but not description), will update. Thanks |
Issue being fixed or feature implemented
In v1.6.2, we reverted change introducing CPU performance optimizations for x86-64 architecture.
We want these optimizations to be enabled in final release image.
We will require CPUs to have microarchitecture level x86-64-v3 (Haswell and newer).
We will still keep unoptimized code in debug images (the ones with
-debugsuffix) as a fallback mechanism.What was done?
Enabled CPU optimizations for release images in:
How Has This Been Tested?
Started drive-abci inside userspace QEMU emulation
Test scenario
Release: test that should fail on usupported x86-64 CPUs
If this test fails, it means your CPU is not supported.
This test should always succeed for all architectures different than x86-64 (I mean
arm).As a result:
Debug: test that should always pass
This test should always pass.
As a result, you should see the version and number 0 at the end of output
Results
x86-64-v1(KVM model: kvm64)Illegal instructionx86-64-v2(KVM model: Nehalem)Illegal instructionx86-64-v3(KVM model: EPYC)Breaking Changes
Dash Platform nodes must be compatible with x86-64-v3 ABI compatibility level 3(x86-64-v3). It translates into Intel Haswell CPU and newer.
Nice, but not normative ,table to help select a CPU: https://www.qemu.org/docs/master/system/i386/cpu.html#abi-compatibility-levels-for-cpu-models
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
x86_64-unknown-linux-gnuwith updated compilation flags.Bug Fixes
x86_64-unknown-linux-musltarget to enhance performance.Documentation
tokio_unstable.