core: optimize using integer down-counter#157686
Conversation
Port the candidate check optimization from upstream to track the loop via an integer down-counter rather than pointer boundaries. This eliminates byte-at-a-time loops and improves LLVM register optimization and latency for short needles. Source: BurntSushi/memchr@e77f0bf Mentored-by: Usman Akinyemi <[email protected]> Signed-off-by: Md Dilshad Azam <[email protected]>
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @clarfonthey (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
This looks reasonable to me, although: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…g-search, r=<try> core: optimize using integer down-counter
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (cc78822): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 2.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.1%, secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 525.47s -> 518.973s (-1.24%) |
|
Yeah, doesn't actually seem like this is an improvement, at least on the current compiler + LLVM version. |
|
@clarfonthey I was trying to address the issue #157593 in which the implementation of SIMD search string was pulled from memchr library which itself got an improvement a few years ago BurntSushi/memchr@e77f0bf. So As mentioned in the issue, I pulled in the improvement here as well. Seems like it did not turn out to be as anticipated. Am I missing something? cc. @BurntSushi |
|
Not sure. I do benchmarks based on wall clock time, not instruction count. Instruction count is a good first approximation, but it's not obvious to me that it should win the day here. I'm not familiar with std's benchmarking metholody, but is there a way to run benchmarks based on the actual thing we care about? That is, wall clock timing. Not instruction count. (The memchr improvements were measured by wall clock time.) |
|
The main issue is that wall clock time is unfortunately very finicky with CI, so, using cachegrind is much more reliable. By the estimated cycle count, which is effectively an estimated wall clock time, it's just a unilateral performance degradation too, so, I'm not 100% sure this is currently working on the latest LLVM version. I would say this kind of thing would probably be best combined with a codegen test to verify that vectorisation is actually happening as we expect, since I'm not 100% sure this version of the code is doing that on the desired targets. |
|
Also it is quite surprising that the wall clock time is also showing performace degradation https://perf.rust-lang.org/compare.html?start=d56483a91d6cf5041351a3208b8d08f98f0c8b56&end=cc7882217fdf82f2f11be804b747f0244f27a55a&stat=wall-time What I do not understand is the perf degradation rather than improvement. The change in logic clearly suggests improvement in performance. |
|
Yeah for sure, instruction count in CI makes a lot of sense. I definitely get that aspect. I re-ran the memchr benchmarks to gauge things here. This is very different than the initial measurements a few years ago: The gap is just a lot smaller than it was before. Not to say that a non-improvement in Anyway, I'm out of ideas here. I'd still try wall clock measurement personally. But it might just be the case that llvm has gotten better over the intervening years. |
I think this is a flag for "let's look at the actual Assembly generated." The critical path should be very small in the SIMD routine, so it shouldn't be too difficult to trace through. |
|
Yeah, we do already have codegen tests for this sort of thing, so, I think it would be fair to add one here too. It should track the LLVM IR generated and we can trust that vector ops will turn into the right SSE/LX/NEON instructions. |
|
@rustbot author (mostly just to clarify more needs to be done; please feel free to continue asking questions!) To provide a bit more info, the codegen tests I was talking about are here: https://github.com/rust-lang/rust/tree/main/tests/codegen-llvm The folder structure here is mostly arbitrary, but I do feel like we should be having codegen tests for this anyway, since this is the kind of thing that can break whenever LLVM is upgraded, and it's mostly about just finding the right code that LLVM will autovectorise for us. |
|
Reminder, once the PR becomes ready for a review, use |
| @@ -1951,59 +1927,55 @@ fn simd_contains(needle: &str, haystack: &str) -> Option<bool> { | |||
| #[inline] | |||
| unsafe fn small_slice_eq(x: &[u8], y: &[u8]) -> bool { | |||
There was a problem hiding this comment.
unsure: how small is small here? We have https://doc.rust-lang.org/std/intrinsics/fn.raw_eq.html that we use for known-size things, so I wonder if there's a place to try something like just listing every possible length in a match and letting the compiler chew on that.
There was a problem hiding this comment.
Alternatively, I wonder if an assert_unchecked plus a call to https://doc.rust-lang.org/std/intrinsics/fn.compare_bytes.html here would give LLVM to freedom to handle it well.
Specifically the advantage of memcmp is that it communicates the "this is readable and not undef" which offers various kinds of freedom. (Though sadly there's no memcmp.inline option the way there is llvm.memcpy.inline.)
There was a problem hiding this comment.
Reading the code that calls this, it appeared that the answer was 0-15 bytes, but not 100% sure on that number.
There was a problem hiding this comment.
That sounds right to me. And in practice I bet it's smaller, but that requires some kind of data analysis on usage. i.e., Smaller than a pointer is I bet very common.
There was a problem hiding this comment.
Very interested to see how other approaches fair here. I have less flexibility in memchr due to not being able to use unstable APIs, but a match expression would be an interesting experiment. (In memchr, the lengths might go up to 32 or something though.)
There was a problem hiding this comment.
Here's a phrasing that's 100% stable but still does the raw_eqs, in case you're interested in playing with it @mazam-97 https://rust.godbolt.org/z/s8vGrzxxo
|
As an overall note here, LLVM is currently working on being able to auto-vectorize short-circuiting loops that it didn't used to be able to do. I don't know what it needs to do that -- the historical trouble is that it doesn't know which parts are readable when the rust code short-circuits -- but as a result the other thing you could consider here is looking at the broader loop and whether there's now a way to convey the important part to LLVM so it can do this in a smarter way than we can ever realistically write in libcore. |
Are you saying this with regard to the perf report? Those numbers are not collected in CI, they are collected on a dedicated physical machine that has specific settings applied to stabilize its behavior.
The significance factors listed in the rightmost column suggest that these are more likely noise than anything real. I think the right conclusion from the rustc-perf report is "no effect", and I would defer to a dedicated benchmark suite for this code path, and a codegen test sounds like a good idea. |
This is not something I knew about; I was under the impression cachegrind was chosen specifically to avoid environment noise. That's good to know. |
|
rustc-perf is not really useful for evaluating library performance changes in general. |
|
|
View all comments
Fix: #157593