Repository navigation
Use AVX-512 streaming instructions to increase write throughput - #35
olivereanderson wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an AVX-512 streaming page-write path with runtime detection and a --no_avx option, a shared constants header, an x86_64-only streaming implementation, refactors memtouch write dispatch to call the streamer or fallback, and updates Meson to build/link new static libraries and AVX-specific flags. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker Thread
participant PreRun as pre_run()
participant MainLoop as Main Write Loop
participant WritePages as write_pages()
participant SIMD as streaming_write_pages_avx512()
participant Fallback as fallback_write_loop()
Worker->>PreRun: start
PreRun->>PreRun: detect AVX-512F & apply --no_avx
PreRun-->>Worker: set avx512_streaming flag
MainLoop->>WritePages: write_pages(page_start, num_pages)
alt avx512_streaming == true
WritePages->>SIMD: dispatch(mem_base, page_start, num_pages)
SIMD->>SIMD: compute iterations (num_pages * PAGE_SIZE/(64*4))
SIMD->>SIMD: prepare 512-bit vector with PATTERN
SIMD->>SIMD: perform four _mm512_stream_si512 stores per iter
SIMD->>SIMD: _mm_sfence()
SIMD-->>WritePages: return
else
WritePages->>Fallback: perform page-by-page writes (loop/memset)
Fallback-->>WritePages: return
end
WritePages-->>MainLoop: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
10981d0 to
4d34361
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
meson.build (1)
1-97:⚠️ Potential issue | 🟡 MinorThis file still fails clang-format-check in CI.
Please apply the repo formatting rules here before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meson.build` around lines 1 - 97, The meson.build file fails the repo's clang-format-check; run the repository formatter (e.g., pre-commit / clang-format with the repo config) on meson.build and commit the formatted result so CI passes. Focus on normalizing whitespace/indentation, line breaks and trailing commas in the cpp_flags array, write_pages_avx512_lib/static_library blocks, and the executable(...) stanza (references: cpp_flags, write_pages_avx512_lib, consts_lib, executable) so the file matches the project's formatting rules.
🧹 Nitpick comments (1)
write_pages_avx512.cpp (1)
10-11: Enforce the page-size divisibility invariant at compile time.The TODO should be a
static_assertto prevent silent partial-write behavior if constants change later.Proposed refactor
void streaming_write_pages_avx512(void* mem_base, uint64_t page_start, uint64_t num_pages) { - // Write 4 * 64 bytes per iteration until we are done - // TODO: Compile time assert that PAGE_SIZE is indeed divisible by 64 * 4 + // Write 4 * 64 bytes per iteration until we are done + static_assert((PAGE_SIZE % (64 * 4)) == 0, "PAGE_SIZE must be divisible by 256 bytes"); auto num_iterations = num_pages * (PAGE_SIZE / (64*4));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@write_pages_avx512.cpp` around lines 10 - 11, Add a compile-time check that PAGE_SIZE is divisible by (64*4) to prevent silent partial-write behavior: replace the TODO with a static_assert using PAGE_SIZE and the divisor (64*4), and keep the existing calculation of num_iterations = num_pages * (PAGE_SIZE / (64*4)); reference PAGE_SIZE and num_iterations (and the expression (64*4)) so the static_assert is colocated near where num_iterations is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@memtouch.cpp`:
- Around line 57-61: There are clang-format/style and a small syntax issue in
the preprocessor block that sets avx512_streaming: add the missing semicolon
after "avx512_streaming = false", ensure the `#if/`#else/#endif block formatting
matches project style, then run clang-format over memtouch.cpp to fix all
formatting violations (also address the other changed regions referenced around
the avx512_streaming logic and the areas corresponding to the earlier noted
hunks). Keep the variable name avx512_streaming and the
__builtin_cpu_supports("avx512f") call intact while correcting punctuation and
formatting.
- Around line 57-61: The non-x86 conditional branch is missing a semicolon
causing a compile error; in the `#else` branch set avx512_streaming = false; (add
the trailing semicolon) so the assignment to the variable avx512_streaming in
the preprocessor branch that contains __x86_64__ / else is syntactically
correct.
In `@write_pages_avx512.cpp`:
- Around line 3-8: The file unconditionally includes the x86-only header
<immintrin.h>, which breaks builds on non-x86 targets; move the `#include`
<immintrin.h> inside the existing architecture guard so it is only included when
__x86_64__ is defined and thus only used by the streaming_write_pages_avx512
function and any x86-specific code.
---
Outside diff comments:
In `@meson.build`:
- Around line 1-97: The meson.build file fails the repo's clang-format-check;
run the repository formatter (e.g., pre-commit / clang-format with the repo
config) on meson.build and commit the formatted result so CI passes. Focus on
normalizing whitespace/indentation, line breaks and trailing commas in the
cpp_flags array, write_pages_avx512_lib/static_library blocks, and the
executable(...) stanza (references: cpp_flags, write_pages_avx512_lib,
consts_lib, executable) so the file matches the project's formatting rules.
---
Nitpick comments:
In `@write_pages_avx512.cpp`:
- Around line 10-11: Add a compile-time check that PAGE_SIZE is divisible by
(64*4) to prevent silent partial-write behavior: replace the TODO with a
static_assert using PAGE_SIZE and the divisor (64*4), and keep the existing
calculation of num_iterations = num_pages * (PAGE_SIZE / (64*4)); reference
PAGE_SIZE and num_iterations (and the expression (64*4)) so the static_assert is
colocated near where num_iterations is computed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61ee0765-2f21-4628-a3f6-8fb35fc3bced
📒 Files selected for processing (5)
consts.hppmemtouch.cppmeson.buildwrite_pages_avx512.cppwrite_pages_avx512.hpp
| auto page = n + pages_read + pages_written; | ||
| write_page(page); | ||
| } | ||
| */ |
There was a problem hiding this comment.
we can remove the old code. No need to keep it
There was a problem hiding this comment.
I forgot about that 😄
| @@ -50,11 +54,21 @@ class WorkerThread { | |||
| stats() {} | |||
There was a problem hiding this comment.
would be good to default initialize avx512_streaming to false here. This way, you can also remove the #else path in pre_run()
There was a problem hiding this comment.
Wait, even better would be to do avx512_streaming(!!__builtin_cpu_supports("avx512f")), wouldn't it?
There was a problem hiding this comment.
The compiler also complains about this :)
../memtouch.cpp: In constructor ‘WorkerThread::WorkerThread(unsigned int, bool, unsigned int, unsigned int, uint64_t)’:
../memtouch.cpp:44:5: warning: ‘WorkerThread::avx512_streaming’ should be initialized in the member initialization list [-Weffc++]
44 | WorkerThread(unsigned id_,
| ^~~~~~~~~~~~
There was a problem hiding this comment.
Changed to __builtin_cpu_supports("avx512f") && (!no_avx) in the constructor.
| void streaming_write_pages_avx512(void* mem_base, uint64_t page_start, uint64_t num_pages) { | ||
| (void) mem_base; | ||
| (void) page_start; | ||
| (void) num_pages; |
There was a problem hiding this comment.
This function should panic because we don't expect it to be actually called, right?
There was a problem hiding this comment.
Yes,
Does C++ have something equivalent to Rust's unreachable! (or at least panic! ) ?
There was a problem hiding this comment.
There is __builtin_unreachable but I would not use it here because the the compiler simply does not generate an function epilogue. Hence, when the function is called at runtime, weird stuff happens.
I would suggest to do somthing like
printf("this function should not be called\n");
__builtin_trap() // generates #UD
There was a problem hiding this comment.
You can also use
std::abort()
which raises SIGABRT
There was a problem hiding this comment.
std::_Exit(-1) is also an alternative
There was a problem hiding this comment.
Changed to std::abort().
|
|
||
|
|
||
| void write_pages(uint64_t page_start, uint64_t num_pages) { | ||
| #if defined (__x86_64__) |
There was a problem hiding this comment.
No need to #if defined this here because you already did this in streaming_write_pages_avx512 and avx512_streaming should always be false for architectures that do not support it
There was a problem hiding this comment.
Yeah good catch! I initially did the logic inline and forgot to remove it afterwards.
There was a problem hiding this comment.
Removed it now, but maybe technically it is better to keep it?
Other platforms will have to check against a trivially false branch on every invocation if the compiler isn't able to figure this out.
|
Nice improvement, I left a few remaks. |
|
I will check the read-bandwidth issue on my systems and report back |
|
Also, it would be really nice if we guard this behind a parameter, at least to opt out. This will make the comparison between avx and non-avx simpler because we do not need to recompile the binary. Maybe |
4d34361 to
7c40fcc
Compare
Done, you can now disable the avx512f path via Will address the other remarks later. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
memtouch.cpp (1)
79-84: Batch the warmup writes too.When
avx512_streamingis enabled, this loop callsstreaming_write_pages_avx512()once per page, and that routine fences once per call inwrite_pages_avx512.cpp:20-23. That throws away a lot of the new write-path gain and makes--onceslower than the steady-state path.♻️ Suggested refactor
- for (uint64_t page{0}; page < num_pages; ++page) { - if (terminate) { - break; - } - write_pages(page, 1); - } + constexpr uint64_t warmup_batch_pages = 64; + for (uint64_t page{0}; page < num_pages and not terminate; + page += warmup_batch_pages) { + auto pages_this_batch = + std::min<uint64_t>(warmup_batch_pages, num_pages - page); + write_pages(page, pages_this_batch); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@memtouch.cpp` around lines 79 - 84, The warmup loop currently calls write_pages(page, 1) per page which, when avx512_streaming is enabled, invokes streaming_write_pages_avx512() and triggers a fence per page; change the loop to batch multiple pages into a single write_pages call so the AVX512 streaming path is called once per batch (reducing fence frequency). Modify the loop in memtouch.cpp to accumulate a batch size (e.g., up to a constant BATCH_PAGES or until terminate/num_pages boundary) and call write_pages(batch_start, batch_count) for each batch; adjust or overload write_pages/streaming_write_pages_avx512 as needed so it accepts a page count and only performs the fence once per batch while preserving the terminate check between batches.meson.build (1)
65-73: Modelconstsas a header-only dependency.
consts.hpponly exports inline constexpr values, so this target has nothing to compile or link. Wrapping it instatic_library()adds an unnecessary archive step and can be brittle across Meson/toolchain combinations;declare_dependency(sources: ..., include_directories: ...)is the cleaner shape here.♻️ Suggested refactor
-# Shared constants -consts_lib = static_library( - 'consts', - sources: ['consts.hpp'], - cpp_args: cpp_flags, - link_args: linker_flags -) - -consts_lib_dep = declare_dependency(link_with: consts_lib) +# Shared constants +consts_lib_dep = declare_dependency( + include_directories: include_directories('.'), + sources: ['consts.hpp'], +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meson.build` around lines 65 - 73, The target `consts_lib` is being created as a static_library even though `consts.hpp` is header-only; replace the static_library + declare_dependency pattern with a single header-only dependency created by declare_dependency(sources: ['consts.hpp'], include_directories: include_directories('.')) and assign that to `consts_lib_dep` (remove `consts_lib` and its static_library call and the link_with usage). Ensure any consumers use `consts_lib_dep` as the dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@memtouch.cpp`:
- Around line 18-22: The xmmintrin.h include and the runtime AVX-512 check are
x86-specific and must be guarded: move the `#include` <xmmintrin.h> into the
existing `#if` defined(__x86_64__) block that already wraps
<write_pages_avx512.hpp>, and change the constructor initialization of
avx512_streaming (where __builtin_cpu_supports("avx512f") is called) so that the
call is only compiled under `#if` defined(__x86_64__) and avx512_streaming
defaults to false on non-x86_64 targets; update references in memtouch.cpp to
use the guarded initialization so non-x86 builds do not reference
__builtin_cpu_supports or xmmintrin.h.
---
Nitpick comments:
In `@memtouch.cpp`:
- Around line 79-84: The warmup loop currently calls write_pages(page, 1) per
page which, when avx512_streaming is enabled, invokes
streaming_write_pages_avx512() and triggers a fence per page; change the loop to
batch multiple pages into a single write_pages call so the AVX512 streaming path
is called once per batch (reducing fence frequency). Modify the loop in
memtouch.cpp to accumulate a batch size (e.g., up to a constant BATCH_PAGES or
until terminate/num_pages boundary) and call write_pages(batch_start,
batch_count) for each batch; adjust or overload
write_pages/streaming_write_pages_avx512 as needed so it accepts a page count
and only performs the fence once per batch while preserving the terminate check
between batches.
In `@meson.build`:
- Around line 65-73: The target `consts_lib` is being created as a
static_library even though `consts.hpp` is header-only; replace the
static_library + declare_dependency pattern with a single header-only dependency
created by declare_dependency(sources: ['consts.hpp'], include_directories:
include_directories('.')) and assign that to `consts_lib_dep` (remove
`consts_lib` and its static_library call and the link_with usage). Ensure any
consumers use `consts_lib_dep` as the dependency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69b6a097-4ff9-4a9a-ad5d-302f8d1fe3b3
📒 Files selected for processing (5)
consts.hppmemtouch.cppmeson.buildwrite_pages_avx512.cppwrite_pages_avx512.hpp
✅ Files skipped from review due to trivial changes (2)
- write_pages_avx512.hpp
- consts.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- write_pages_avx512.cpp
7c40fcc to
e5107cf
Compare
| @@ -68,7 +80,7 @@ class WorkerThread { | |||
| if (terminate) { | |||
There was a problem hiding this comment.
I am wondering whether terminate could be hoisted out of the loop?
The value of terminate should not be able to change in between loop iterations right?
It would be nice to replace this entire loop with
if !(terminate) {
write_pages(0, num_pages);
}
There was a problem hiding this comment.
I think the problem is that the warmup operation can take multiple seconds or even a minute (assume you write 14tb) and should be therefore abortable early. Therefore, we should keep it as is. terminate is set to true indirectly from the signal handler
There was a problem hiding this comment.
So you are saying that the value of the terminate bool can change between loop iterations?
If so should we not be doing atomic loads here?
There was a problem hiding this comment.
So you are saying that the value of the terminate bool can change between loop iterations?
yes.
If so should we not be doing atomic loads here?
Would technically be more correct, but in the worst case we would do simply one iteration more then necessary. You could fix this in a dedicated commti if you want.
There was a problem hiding this comment.
but in the worst case we would do simply one iteration more then necessary.
No, in the worst case we have UB. It certainly would be in Rust and I suspect the same to be true in C++.
There was a problem hiding this comment.
Ah yes, thanks! Of course. I am not used to think about UB in C++ (barely programming with it these days) 😁
More information: https://stackoverflow.com/a/78876575
Using a atomic variable would be the safer approach: std::atomic<int> x;
There was a problem hiding this comment.
Then to conclude: we leave this as is here and I address the existing UB in a follow up PR.
Signed-off-by: Oliver Anderson <oliver.anderson@cyberus-technology.de> On-behalf-of: SAP oliver.anderson@sap.com
e5107cf to
5039408
Compare
There was a problem hiding this comment.
Nice addition! My review was done with the help of Claude/Sonnet 4.6: I have
some comments. Also in general, your commit messages should explain why you use AVX512 and what the estimated performance gains are.
write_pages_avx512.cpp, line 12 — must-fix
The TODO is trivially resolvable since PAGE_SIZE is constexpr:
static_assert(PAGE_SIZE % (64 * 4) == 0, "PAGE_SIZE must be divisible by 256");write_pages_avx512.cpp, line 15 — nit
C-style cast; use static_cast<char*>(mem_base) for consistency with the rest of the codebase.
meson.build, line 62 — should-fix
consts_lib is declared as a static_library with only a .hpp source. Meson will
produce an empty archive and the resulting link_with dependency on all consumers is
unnecessary overhead. Replace with a plain declare_dependency(include_directories: ...)
— no static_library, no linker dependency.
meson.build, line 86 — nit
Missing blank line between write_pages_avx512_lib_dep = declare_dependency(...) and the
following comment, inconsistent with the rest of the file.
memtouch.cpp, --no_avx argument — nit
Consider --no_avx512 to make clear this targets AVX-512 specifically and leaves room
for future SIMD paths. The existing flags use underscores consistently so the separator
style isn't the issue.
memtouch.cpp, pre_run() alignment assert — nit
The assert is gated on avx512_streaming but mmap(MAP_ANONYMOUS) already guarantees
page-granularity alignment (≥ 4096 bytes >> 64 bytes), so it can never fire. Either drop
it or make it unconditional with a comment referencing the mmap guarantee.
consts.hpp — nit
inline static constexpr at namespace scope is redundant: constexpr implies inline
since C++17. Use plain constexpr (or inline constexpr if you want to be explicit,
though that too is redundant at namespace scope).
Suggested commit split
One broader suggestion: the patch bundles a build system refactor, a new SIMD backend,
and a runtime dispatch mechanism into a single commit, which makes it harder to bisect
and review. I'd suggest splitting into three commits:
Commit 1: build: extract constants into shared consts.hpp
Move PAGE_SIZE, PATTERN, and DEFAULT_STAT_IVAL out of memtouch.cpp into a new
consts.hpp header so they can be shared with future translation units without pulling
in all of memtouch.cpp. No functional change.
Commit 2: memtouch: add AVX-512 non-temporal write backend
Add write_pages_avx512.cpp / .hpp implementing streaming_write_pages_avx512().
The function writes 256 bytes per loop iteration using four _mm512_stream_si512 stores
(non-temporal, bypassing the cache) and issues _mm_sfence on exit to restore store
ordering. The translation unit is compiled separately with -mavx512f so the flag does
not leak into the rest of the build. On non-x86 targets the function stub aborts, guarded
by the #if defined(__x86_64__) call-site check in memtouch.cpp.
Commit 3: memtouch: use AVX-512 streaming writes when available
Wire up the new backend via runtime CPU feature detection
(__builtin_cpu_supports("avx512f")). Workers opt in at construction time and fall back
to the existing memset-based path when AVX-512 is absent or --no_avx512 is passed.
Expected benefit: non-temporal stores bypass the LLC on write, reducing memory write
latency and avoiding cache pollution for this write-dominated workload.
|
Closing in favour of #37 |
| static constexpr int PATTERN(0xff); | ||
| static constexpr int DEFAULT_STAT_IVAL(1000); | ||
|
|
||
| static std::atomic<bool> TERMINATE; |
There was a problem hiding this comment.
please don't use upper case for variables, even if they are global. Just call it terminate or global_terminate if you want to make it more obvious.
There was a problem hiding this comment.
Also, you can auto initialize this to false with:
static std::atomic<bool> global_terminate {false};
This saves you from initializing it in main()
Non-temporal stores are known to help in the case of bulk write workloads. This PR thus replaces the write_page loops with a function issuing
VMOVNTDQinstructions in a loop (storing 4zmmregisters to memory with a non-temporal hint per iteration) wheneveravx512fis available.Benchmarks
TL;DR: write_mibps increases by a factor between roughly 1.7 and 4x depending on the parameters, but read_mibps suffers by roughly 15-30% on Intel Sapphire Rapids + Granite Rapids and is roughly halved in the 50% read case on AMD Turin (Zen 5).
All benchmarks are performed in a Cloud hypervisor guest with direct Kernel boot.
RW_RATIO = 100
Intel Sapphire Rapids
Before this change:
With this change:
Intel Granite Rapids
Before this change:
After this change:
AMD Turin (Zen 5)
Before this change:
After this change:
RW_RATIO=50
Sapphire Rapids
Before this change:
After this change:
We now have much less consistent numbers, but writes are always much higher and reads always a bit lower.
Granite Rapids
Before this change:
After this change:
We still get roughly 4x more write_mibps and the read_mibps is penalized a bit less than on the Sapphire Rapids.
AMD Turin (Zen 5)
Before this change:
After this change:
We here see that write_mibps is increased by around 4x, but read_mibps is almost halved though.
Implementation details
This is my first time writing C++ and using Meson. There is thus probably a lot of room for improvements!
C++ Code
I branch on a pre-computed bool in the
write_pagesfunction and execute the fast path whenavx512fis available on x86_64 targets. Normally one would perhaps rather use dynamic dispatch through a function pointer here, but I am not sure how one would write that in C++. In this particular case I also expect the performance penalty of including effectively dead code at the beginning of the function call to be rather negligible in comparison to the time spent in the hot write loop.Meson
In order to compile the avx512 code one needs to supply the
avx512fflag to the compiler, but we don't want this to apply to all code because we can then get UB on (x86_64) machines that do not have these features. I thus had to create more static_libraries with separate compiler flags. I hope this is correct, but I am not sure how idiomatic or nice my approach is here at all.One would typically do things a bit differently in a Rust project.
Summary by CodeRabbit
New Features
Chores