Don't implement mem::replace with mem::swap.

[m-ou-se]

swap is a complicated operation, so this changes the implementation of replace to use read and write instead.

See #83019.

I wrote there:

Implementing the simpler operation (replace) with the much more complicated operation (swap) doesn't make a whole lot of sense. replace is just read+write, and the primitive for moving out of a &mut. swap is for doing that to two &mut at the same time, which is both more niche and more complicated (as shown by swap_nonoverlapping_bytes).

This could be especially interesting for Option<VeryLargeStruct>::take(), since swapping such a large structure with swap_nonoverlapping_bytes is going to be much less efficient than ptr::write()'ing a None.

But also for small values where swap just reads/writes using temporary variable, this makes a replace or take operation simpler:

[rust-highfive]

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

[m-ou-se]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit bf27819 with merge 9acd471bcf643c584381c0ff22e4d38a7617f313...

[eddyb]

FWIW I just tested this PR against Rust-GPU and it appears to handle the for i in 0..n motivation, from #83019's description.

[bors]

☀️ Try build successful - checks-actions
Build commit: 9acd471bcf643c584381c0ff22e4d38a7617f313 (9acd471bcf643c584381c0ff22e4d38a7617f313)

[rust-timer]

Queued 9acd471bcf643c584381c0ff22e4d38a7617f313 with parent 5c6d3bf, future comparison URL.

[nagisa]

since swapping such a large structure with swap_nonoverlapping_bytes is going to be much less efficient than ptr::write()'ing a None.

I don't believe that's true – assigning a None is a disciminant write, but ptr::write will need to copy over all the size_of::<Option<VeryLargeStruct>>() bytes of the type regardless of whether the value is a None or not, won't it?

[nagisa]

x86_64, aarch64 assembly-level comparison.

[m-ou-se]

@nagisa Nope, that just writes the tag of the Option, and nothing else. (Just like write()'ing a MaybeUninit::uninit() writes nothing at all.)

But looks like llvm is also smart enough to do that with the more compicated swap code: https://godbolt.org/z/TM1bcv

[khyperia]

I don't believe that's true – assigning a None is a disciminant write, but ptr::write will need to copy over all the size_of::<Option<VeryLargeStruct>>() bytes of the type regardless of whether the value is a None or not, won't it?

x86_64, aarch64 assembly-level comparison.

Looking at the x86_64 assembly, changing it to be this shows off how the ptr::write version optimizes the None case, where the swap version doesn't.

pub fn replace_large(dest: &mut Option<[u8; 127]>) -> Option<[u8; 127]> {
    replace(dest, None)
}

But looks like llvm is also smart enough to do that with the more compicated swap code

Not smart enough in all cases, I guess? No idea how this case is different than yours, though.

edit: discovered this weird thing, I mistakenly thought it was relevant but I missed something ugh. big shrug~

godbolt link

pub fn speedy_fast(x: &mut Option<[u64; 10]>) -> Option<[u64; 10]> {
    std_replace(x, None)
}
// copied from std::mem::replace, just to be clear what's happening (same behavior with std::mem::replace)
fn std_replace(dest: &mut Option<[u64; 10]>, mut src: Option<[u64; 10]>) -> Option<[u64; 10]> {
    std::mem::swap(dest, &mut src);
    src
}

// manually inlined, should be the same? but isn't
pub fn big_slow_write(x: &mut Option<[u64; 10]>) -> Option<[u64; 10]> {
    let mut blah = None;
    std::mem::swap(x, &mut blah);
    blah
}

[rust-timer]

Finished benchmarking try commit (9acd471bcf643c584381c0ff22e4d38a7617f313): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[m-ou-se]

🎉

Quite a significant improvement! Everything is green, except for one +174% that pushed one average into the red. I suppose that's just a random outlier:

[m-ou-se]

r? @eddyb

[klensy]

Can it rerun perf without try?

@rust-timer queue include=46449

[rust-timer]

Insufficient permissions to issue commands to rust-timer.

[nagisa]

@rust-timer queue include=46449

I sure have permissions, lets see what happens.

EDIT: Looks like no, you can't rerun perf withouta another try build :(

[nagisa]

If nothing else, optimizing a simpler chunk of code is easier on the backend. While I sympathize with @SimonSapin's point about centralization of the unsafe code, I think replace and similar such heavily used functions benefit from such targeted simplifications a… lot.

The instruction count/walltime regression on the incr-patched: empty 3072 test appears to be almost entirely within LLVM's LLVM_lto_optimize and similar areas, so I don't expect much difference at all from a rerun. My guess here is that LLVM simply ends up making different decisions (e.g. giving replace or something similar a higher inline score) when optimizing the newly patched code and thus hitting one of its non-linear areas of code with a significantly larger amount of code.

[bors]

☀️ Try build successful - checks-actions
Build commit: 6537b60dde68a3362e54e06e38e45445cd1e920d (6537b60dde68a3362e54e06e38e45445cd1e920d)

[rust-timer]

Queued 6537b60dde68a3362e54e06e38e45445cd1e920d with parent 0cc64a3, future comparison URL.

[rust-timer]

Finished benchmarking try commit (6537b60dde68a3362e54e06e38e45445cd1e920d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

[nagisa]

Well, we're good to go, I guess!

@bors r+

[bors]

📌 Commit bf27819 has been approved by nagisa

[SimonSapin]

For what it’s worth I wasn’t saying in #83019 that we shouldn’t change anything, only that the current situation was not entirely accidental.

These macro-level results look great. Since this effectively undoes #40454 for replace it might be interesting to also look at micro-level effects, especially for large values. Unfortunately I don’t find code to reproduce the benchmarks in #40454.

[bors]

⌛ Testing commit bf27819 with merge 46a934a...

[bors]

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 46a934a to master...

[jordens]

Thanks for this fix! Example where this (I think) was painful: taking and replacing packet size data in Options:
https://godbolt.org/z/5dPh8eoWa

[m-ou-se]

Thanks for this fix! Example where this (I think) was painful: taking and replacing packet size data in Options:
https://godbolt.org/z/5dPh8eoWa

Nice, that's a huge difference! Even with --opt-level set to z or s, it makes a big difference.