-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
optimized memcpy #18912
optimized memcpy #18912
Conversation
Nice work! Could you share the benchmark script? I am really curious to see how those aligned loads/stores are emitted. |
The benchmark script is just a pretty simple loop copying from one buffer to the other a bunch of times for each source alignment offset. I haven't done anything clever in the micro benchmark. The reason aligned vector moves are emitted is the use of Here's the micro benchmark (doesn't look like I can colour it inside the fold):pub fn main() !void { const allocator = std.heap.page_allocator; |
Interesting. I have attempted to re-produce this and I see 2, maybe 3 issues.
I recommend you use something like: dest[0..v].* = @as(*const @Vector(v, u8), @alignCast(@ptrCast(aligned_src[offset..][0..v]))).*; to generate your aligned vector loads, 😉 |
I am not convinced this is the case. It is trivial to produce a longer sequence of aligned move operations by unrolling the loop - though this does neccesitate extra cleanup code - and when I did this in the past it did not have much impact on performance. Unless I get benchmark results clearly showing a win that seems generalizable I wouldn't want to do it; forcing this sort of unrolling seems more likely to trip up the optimizer and may only positively impact performance on the machine you run the benchmark on while degrading performance on other machines.
If you let me know precisely how you compiled/ran it I will try on my machine as well. Particular things of interest would be how many iterations you did, the copy length, target cpu features, and optimize mode.
Can you be more specific about what you did when you saw worse performance relative to master so I can investigate? I have also used compiling the zig compiler itself as a test and saw a minor (not really sure it was outside uncertainty) improvement in compile time.
Can you post your benchmark? I'm not sure what you mean by "...and also cannot replicate it" along with "Very comparable results to the benchmark script your provided." - those read as contradictory statements to me, so I must be misinterpretting something. |
Looks like this somehow broke a bunch of non-x86_64 targets in the linux CI. I wonder if on those targets LLVM is making memcpy produce a call to itself... |
I assume you mean |
It looks like this is the issue, at least for wasm32-wasi. I checked a If anyone knows of a way to trace wasm code produced back to source lines, similar to |
I suspect zig’s memcpy could get a whole lot faster than 30%-ish. I think it’s hard to rule out measurement noise without also having a complete benchmark snippet for others to run and on larger amounts of data. There must already be comprehensive memcpy benchmarks online somewhere you could copy from |
For small sizes this is certainly true, but for large copies I'm a bit skeptical, at least not without using inline asm with something like |
I've made a bunch of improvements yielding both better performance and smaller code size. The table in the top post has been updated. Edit: Not sure what the problem with riscv64-linux is in the CI - I've disassembled it and memcpy and is not doing a recursive call. |
This is wrong, recursive |
81a4c1c
to
076afc6
Compare
The windows CI failure is |
@dweiller I just ran into that in a different PR as well, seems to be a fluke. |
The windows CI failure seems to be the compiler running out of memory - not sure why this would happen, it shouldn't be caused by this PR. |
A decent amount of the code could be simplified using comptime into something like: // shared (dst, src)
copy(blk, offsets):
for i in offsets:
dst[i..][0..blk].* = src[i..][0..blk].*
memcpy(len):
if len == 0: return
if len < 4: return copy(1, {0, len/2, len-1})
v = max(4, suggestVectorSize(u8) or 0)
inline for n in ctz(4)..ctz(v):
if len <= 1 << (n+1): // @expect(false)
return copy(1<<n, {0, len - (1<<n)})
for i in 0..(len / v): copy(v, .{ i * v }) // 4-way auto-vec
copy(v, .{ len - v }) But I think (at a higher level) we should go all the way: Facebook's folly memcpy is heavily optimized but also serves as their Interestingly. they never use aligned loads since it doesn't seem worth it. Copy-forward does both load/store unaligned, but copy-backward uses aligned stores with The general strategy of having memcpy & memmove be the same thing would be nice (perf & maintenance wise). Fast |
This looks pretty nice, I'll try it out.
Our memcpy assumes that src/dst don't overlap, so a change like that would be a (non breaking, but significant) change in semantics that would affect performance.
On my machine, IIRC, the aligned ops did affect performance, but this would also be something machine dependent. I have seen that the current wisdom seems to be that modern x86_64 doesn't really care (but for some reason haven't seen when this started being the case), but what about other platforms? At least for a general algorithm, I would think we should use aligned ops, and if/when we want to individually optimise different platforms we could use unaligned ops. I did also try it unrolling with prefetch, but didn't want to over-optimise for my machine - I can't recall how much difference it made for me.
I did some research into
I do actually have a memset branch locally as well - I could include it in this PR, but I haven't put as much work into it yet. |
I rather mean to keep memcpy, the function with noalias and such, but have it just call memmove internally. This should keep the noalias optimizations applied by compiler replacing memcpy, but keep the vector-based / branch-optimized version at runtime.
Was this in relation to the results above? I think the benchmark could report throughput in cycles/byte rather than ns. This is something used in benchmarks like I only mention this as aligned loads didn't seem to have an effect on other benchmarks so trying to somehow discover/rationalize the difference in the results. TBF, also haven't tested on anything outside avx2, avx512f, and apple_m1.
Indeed looks like it depends on micro-architecture specifically (being finnicky on zen2+). Seemed to be same speed as the normal vectorized loop at least on 5600x and 6900HS.
Yea a separate PR would be best. Just wanted to mention their similarity. |
Ah, okay - I understand now.
I think the benchmark has changed between when I tested (and the memcpy function has quite a bit too) - I'll re-check. Unless I'm missing something, I would be biased to keep the aligned strategy until/unless we diverge implementation based on whether a target has slow unaligned access or not, because I expect the impact on systems that don't care to be minimal (it costs one branch and one vector copy but this is only on large copies which are rare, and the effects of the increased code size), but systems that do have slower unaligned accesses will pay that cost proportional to the size of a long copy. I can have the benchmark report cycles/byte, and do things like core-pinning and disabling frequency scaling next time I work on improving the benchmarking, but I think adding other benchmarks will probably take priority.
One reason, could be the architecture - looking at Agner Fog's tables my machine (Zen 1) has worse latencies/throughputs on unaligned intstructions (at least for integer ones, can't remember at the moment if integer or float instructions were generated). |
It looks like this is not ready for merging then? Can you close it and open a new one when it's ready for review & merging? Otherwise we can use the issue tracker to discuss ideas, plans, strategies, etc. |
I've just done some cleanup of commits and assuming it passes CI, I'm happy for it to be merged. The unchecked boxes are all things that are potential future work and could be broken out into new issues. I would say the real question is what level of effort you want to see put into benchmarking before merging. I can certainly do more on my own - write more synthetic benchmarks, do some proper statistics, check some real-world impacts more carefully (I did see a small benefit in the zig compiler using the c backend, but that was a while ago and I don't remember the details) - but I think any serious benchmarking effort would require help from others (in particular to check on more machines/architectures) and may be better done as part of gotta-go-fast. |
Looking at the table above again - would it be better to not include the optimisations done in this PR on |
I've just run the distribution benchmark to compare the current PR with https://github.com/Rexicon226/zig/tree/memcpy-go-brr (commit 4e0ce4b) which I assume is @Rexicon226's latest memcpy version and got these results on my machine: I'm not sure what benchmark Rexicon was running in the past when he was comparing his version to old versions of this PR, so I can't check those results. |
I think that manually unrolling is not a good idea at present as LLVM further unrolls the copy loops on various targets and does so too aggressively. It would be reasonable to do manual unrolling if we have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tired of waiting for @Rexicon226 so if you address this feedback I'll merge it. Thanks for working on this.
Should I also remove all the |
For compiler_rt it's important that all the intrinsics are lowered to exactly one function. This helps codegen be better since the calls are known leaf calls, and it interacts better with |
The commit f38d7a9 makes the memcpy implementation much more load-bearing, so it's important to rebase on top of latest master. Can you update your perf measurements against latest master? Can you take data points on aarch64 as well as x86_64? |
I'll update and re-measure, but I don't have a aarch64 machine to benchmark on, I'll need someone else willing to run the benchmarks. |
I volunteer, I've got 2 aarch64 systems and even a RISC-V system that can be used. |
The new memcpy function aims to be more generic than the previous implementation which was adapted from an implementation optimized for x86_64 avx2 machines. Even on x86_64 avx2 machines this implementation should be generally be faster due to fewer branches in the small length cases and generating less machine code. Note that the new memcpy function no longer acts as a memmove.
I have rebased and updated the OP to reflect the current state of the PR. Note that this PR now (i.e. commit b7a887f) splits
The benchmark I use in available at https://github.com/dweiller/zig-lib-bench if you can run the 'distrib' benchmark that would be great. If you have any issues feel free to raise issues there. It should be usable as is, but if you give me a few days I can try and make it a bit easier to use. |
Note that FSRM does not imply ERMSB. FSRM just means that you can use |
Between ZSF and I, we have at least skylake, zen2, zen3, zen4, neoverse_n1, and apple_a15. If you provide specific instructions for producing a file for you to interpret and what cpus you need, I can do so. |
Thanks for the rebase and the re-writeup. I'm now happy with this as-is. ARM perf can be a followup investigation. Appreciate your patience with this one, @dweiller. |
I'm not sure what the CI failure on aarch64-windows is - as far as I see in the CI log there is just a exit 5 in the |
Ok - I'll just wait for that PR and rebase when it lands. |
const first = @as(*align(1) const @Vector(32, u8), @ptrCast(s - 32)).*; | ||
const second = @as(*align(1) const @Vector(32, u8), @ptrCast(s - 64)).*; | ||
const third = @as(*align(1) const @Vector(32, u8), @ptrCast(s - 96)).*; | ||
const fourth = @as(*align(1) const @Vector(32, u8), @ptrCast(s - 128)).*; | ||
|
||
@as(*align(32) @Vector(32, u8), @alignCast(@ptrCast(d - 32))).* = first; | ||
@as(*align(32) @Vector(32, u8), @alignCast(@ptrCast(d - 64))).* = second; | ||
@as(*align(32) @Vector(32, u8), @alignCast(@ptrCast(d - 96))).* = third; | ||
@as(*align(32) @Vector(32, u8), @alignCast(@ptrCast(d - 128))).* = fourth; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is not valid, as there is no guarantee that @sizeOf(@Vector(32, u8)) == 32
for every target. Perhaps you meant [32]u8
instead of @Vector(32, u8)
since @sizeOf([32]u8) == 32
is guaranteed for every target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was the pre-existing memmove - I just moved it. I've got a branch with a memmove implementation based on the memcpy from this PR which wouldn't have that issue, but haven't had time to do benchmarking for it yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose the memcpy
implementation in this PR may have a similar issue if std.simd.suggestVectorLength
does not return a suitable length.
assert(@sizeOf(Element) >= @alignOf(Element)); | ||
assert(std.math.isPowerOfTwo(@sizeOf(Element))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All types have a size that is a multiple of its alignment, therefore the first condition isn't checking for anything. If you need a power of two, just use std.math.floorPowerOfTwo
or something.
@Type(.{ .vector = .{ | ||
.child = u8, | ||
.len = vec_size, | ||
} }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Type(.{ .vector = .{ | |
.child = u8, | |
.len = vec_size, | |
} }) | |
@Vector(vec_size, u8) |
This PR does a few things to improve
memcpy
performance:Here is a graph or the performance of master (commit 5cfcb01) and b7a887f in a microbenchmark. The benchmark times
@memcpy
for a specific length and alignment (mod 32) of source and destination 100 000 times in a loop; this is done for all 256 combinations of source and destination alignment and the average time per iteration across all alignment combinations is the reported result.Note that this benchmark is not going to be particularly realistic for most circumstances as branch predictors will be perfectly trained.
In general we want to focus on optimizing small length copies as small lengths are the most frequent—some reported distributions can be found at here and this google paper (LLVM uses these for benchmarking). For small lengths, the above graph indicates between a modest and significant (up to around 3x) improvement, depending on the length.
Here is a graph showing performance across the distributions from the linked paper linked above (taken from the LLVM repo).
While this PR is not focussed on
memmove
I have also changed theReleaseSmall
implementation ofmemmove
, as the new (since f38d7a9)memmove
implementation is not handled well by LLVM inReleaseSmall
mode.For code size of memcpy:
Note that the above column for master contains the size of the
memmove
implementation, thememcpy
function on master compiles to 10 bytes of stack shuffling and a jump tomemmove
.The
ReleaseSmall
size ofmemmove
for b7a887f is 58 bytes for the cpus above.I have only checked the performance of this change on my (x86_64, avx2, znver1) machine—it would be good to check on other architectures as well if someone with one would like to test it. The benchmarks used can be found here—the
average
anddistrib
ones were used to produce the above charts (check out thetools
directory for data generation scripts).A few other notes:
memcpy
andmemmove
no longer share implementationsReleaseSmall
performancerep movsb
for copies over several hundred bytes (I don't have a suitable machine to test this)rep movsb
for all/most cases (I don't have a suitable machine to test this)rep movsb
from x86_64)Other stuff that can be done (either before merging or as follow up issues):
memmove
performance and whether it makes sense to merge/share code withmemcpy
againsee if there is a reasonable way to do aligned vector moves for misaligned case (doubtful without inline assembly)pretty sure this can't be done without@select
taking a runtime mask or using inline assembly