Speed up ptr::replace

[lukas-code]

I noticed, that ptr::replace and mem::replace are implemented differently, even though they should probably be the same.

Looking at the goldbolt output, it seems that mem::replace does only 2 memcpys and ptr::replace does 2 memcpys with an inlined copy in between.

I ran some benchmarks on my machine, in which mem::replace was more than 2x faster than ptr::replace.

Benchmark

Code

#![feature(test)]
#![cfg(test)]

extern crate test;

use rand::Rng;
use std::{mem, ptr};
use test::Bencher;

#[bench]
fn mem_replace(b: &mut Bencher) {
    let mut rng = rand::thread_rng();

    let mut dst = [0u8; 256];
    rng.fill(&mut dst);

    let mut src = [0u8; 256];
    rng.fill(&mut src);

    b.iter(|| {
        let mut dst_copy = dst;
        let old = mem::replace(&mut dst_copy, src);
        (old, dst_copy)
    });
}

#[bench]
fn ptr_replace(b: &mut Bencher) {
    let mut rng = rand::thread_rng();

    let mut dst = [0u8; 256];
    rng.fill(&mut dst);

    let mut src = [0u8; 256];
    rng.fill(&mut src);

    b.iter(|| {
        let mut dst_copy = dst;
        let old = unsafe { ptr::replace(&mut dst_copy, src) };
        (old, dst_copy)
    });
}

Output

test mem_replace ... bench:          11 ns/iter (+/- 0)
test ptr_replace ... bench:          25 ns/iter (+/- 0)

In this PR i have replaced the implementation of ptr::replace to call mem::replace instead of mem::swap.

[rust-highfive]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @kennytm

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

[Dylan-DPC]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit bc0788b with merge fca4bf64b483deb3f9c962caf2ddf63c79124b81...

[bors]

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

[rust-timer]

Queued fca4bf64b483deb3f9c962caf2ddf63c79124b81 with parent 0887113, future comparison URL.

[rust-timer]

Finished benchmarking commit (fca4bf64b483deb3f9c962caf2ddf63c79124b81): comparison url.

Instruction count

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 😿 relevant regression found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.8%	0.8%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions 😿 (primary)	7.4%	7.4%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	7.4%	7.4%	1

Cycles

Results

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions 😿 (primary)	2.8%	2.9%	2
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.8%	2.9%	2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[scottmcm]

cc @m-ou-se, who did #83022 and thus probably has thoughts here.

also cc @RalfJung, who might have thoughts about the typed-ness of this like they did in #97712

[m-ou-se]

Looks good to me, but I'm a bit surprised with the benchmarking results.

[RalfJung]

also cc @RalfJung, who might have thoughts about the typed-ness of this like they did in #97712

This operation takes and returns a T, so it has no choice but to do a typed copy.

It doesn't seem to actually help in rustc though? Not sure how representative that one microbenchmark is.

[RalfJung]

Suggested change

	// and cannot overlap `src` since `dst` must point to a distinct
	// allocated object.
	// and cannot overlap `src` since `src` is a freshly stack-allocated local variable.

Not that we even still need this part of the argument with mem::replace.

[lukas-code]

It probably doesn't improve performance in rustc a lot (at all), because it barely seems to be used in the Rust repository. All uses of ptr::replace I could find are in the implementation of mpsc in the standard library.

rust/library/std/src/sync/mpsc/oneshot.rs

Line 182 in 7036449

None => match ptr::replace(self.upgrade.get(), SendUsed) {

rust/library/std/src/sync/mpsc/oneshot.rs

Line 216 in 7036449

ptr::replace(self.upgrade.get(), prev);

rust/library/std/src/sync/mpsc/oneshot.rs

Line 295 in 7036449

match ptr::replace(self.upgrade.get(), SendUsed) {

rust/library/std/src/sync/mpsc/shared.rs

Line 260 in 7036449

let steals = ptr::replace(self.steals.get(), 0);

rust/library/std/src/sync/mpsc/stream.rs

Line 164 in 7036449

let steals = unsafe { ptr::replace(self.queue.consumer_addition().steals.get(), 0) };

Additionally I found one instance of mem::replace that should probably be a ptr::replace:

rust/library/std/src/thread/local.rs

Lines 812 to 831 in 7036449

	// SAFETY:
	//
	// note that this can in theory just be `*ptr = Some(value)`, but due to
	// the compiler will currently codegen that pattern with something like:
	//
	// ptr::drop_in_place(ptr)
	// ptr::write(ptr, Some(value))
	//
	// Due to this pattern it's possible for the destructor of the value in
	// `ptr` (e.g., if this is being recursively initialized) to re-access
	// TLS, in which case there will be a `&` and `&mut` pointer to the same
	// value (an aliasing violation). To avoid setting the "I'm running a
	// destructor" flag we just use `mem::replace` which should sequence the
	// operations a little differently and make this safe to call.
	//
	// The precondition also ensures that we are the only one accessing
	// `self` at the moment so replacing is fine.
	unsafe {
	let _ = mem::replace(&mut *ptr, Some(value));
	}

[lukas-code]

Due to what?

[JohnCSimon]

ping from triage:
@lukas-code
looks like this PR is still in progress - so returning to author

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@rustbot author

[lukas-code]

Since this doesn't appear to increase performance at all, I'll just close this.