do not mark interior mutable shared refs as dereferenceable

[RalfJung]

My proposed solution to #55005.

[rust-highfive]

r? @wesleywiser

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

[RalfJung]

This will fail a codegen test, but I think try builds don't care?
@bors try

[bors]

⌛ Trying commit 1af98f60f0facb1221bda0b6819791691264d583 with merge 7c7ee4caf159c6825c63c55cde19b5727f1d55ba...

[bors]

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

[nikic]

What information do you hope to gain from a crater run here? If I understood correct, his allows less optimization, not more, so it shouldn't break any unsafe code assumptions?

[RalfJung]

Sorry, I meant "perf experiment", not crater 🤦

[RalfJung]

@rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bjorn3]

@rust-timer build 7c7ee4caf159c6825c63c55cde19b5727f1d55ba

[rust-timer]

Queued 7c7ee4caf159c6825c63c55cde19b5727f1d55ba with parent 37a4225, future comparison URL.

[RalfJung]

Ah, how do I start a perf run when the try build is already done? Sadly I found no documentation for rust-timer commands... the forge only lists the "queue" command.

EDIT: Thanks @bjorn3 :)

[bjorn3]

It is documented at https://github.com/rust-lang/rustc-perf/tree/master/collector#how-to-benchmark-a-change-using-the-benchmark-server as "the harder way".

[rust-timer]

Finished benchmarking commit (7c7ee4caf159c6825c63c55cde19b5727f1d55ba): comparison url.

Instruction count

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: 🎉 relevant improvements found

	mean1	max	count2
Regressions 😿 (primary)	0.6%	0.8%	5
Regressions 😿 (secondary)	0.9%	1.0%	2
Improvements 🎉 (primary)	-0.4%	-2.9%	84
Improvements 🎉 (secondary)	-0.4%	-0.9%	29
All 😿🎉 (primary)	-0.3%	-2.9%	89

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

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

Cycles

Results

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: no relevant changes found

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

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 ↩ ↩² ↩³

[RalfJung]

It is documented at https://github.com/rust-lang/rustc-perf/tree/master/collector#how-to-benchmark-a-change-using-the-benchmark-server as "the harder way".

Ah, so there's a bot and a collector, but the bot command docs are with the collector. No wonder I missed them. ;) I'll add a link to the forge. Thanks!

[RalfJung]

So, uh, this change is more of a perf gain than a loss? That is certainly not what I expected.

My guess would be this is because we emit fewer attributes, and the attribute itself did not help the make the compiler much faster. Not sure how much we can conclude from that.

[RalfJung]

Based on my last comment in #55005, I adjusted this PR to keep dereferenceable in mutable refs but remove it from non-frozen shared refs.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

📌 Commit 35c6dec has been approved by nikic

It is now in the queue for this repository.

[bors]

⌛ Testing commit 35c6dec with merge 848090d...

[bors]

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 848090d to master...

[rust-timer]

Finished benchmarking commit (848090d): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: 🎉 relevant improvements found
• Secondary benchmarks: no relevant changes found

	mean1	max	count2
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-3.1%	-3.9%	2
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-3.1%	-3.9%	2

Cycles

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: 🎉 relevant improvement found

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

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

@rustbot label: -perf-regression

Footnotes

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

2. number of relevant changes ↩ ↩²

[m-ou-se]

It's not entirely clear to me how an implementation of Drop for Arc should look like to make use of this new rule/exception to avoid undefined behavior.

The usual pattern looks something like this:

pub struct Arc<T> {
    ptr: NonNull<ArcData<T>>,
}

struct ArcData<T> {
    ref_count: AtomicUsize,
    data: T,
}

impl<T> Arc<T> {
    …

    fn data(&self) -> &ArcData<T> {
        unsafe { self.ptr.as_ref() }
    }
}

impl<T> Drop for Arc<T> {
    fn drop(&mut self) {
        if self.data().ref_count.fetch_sub(1, Release) == 1 {
            fence(Acquire);
            unsafe {
                drop(Box::from_raw(self.ptr.as_ptr()));
            }
        }
    }
}

Is this fine now?

Or is it a problem that self.data() returns a &ArcData<T> to the whole struct, as part of the same if condition expression that does the fetch_sub operation?

[RalfJung]

Or is it a problem that self.data() returns a &ArcData to the whole struct, as part of the same if condition expression that does the fetch_sub operation?

Yes this is a problem under the rules as written, since the lifetime of ArcData is still ongoing when another thread might deallocate this memory. A raw pointer should be used instead.

It is not a problem under the rules as implemented in Miri (aka Stacked Borrows), where the "dynamic lifetime" of the reference is relevant -- the lifetime of the ref_count part of the reference is extended until fetch_sub returns, so that could still be alive when another thread deallocates the memory, but it is entirely covered by UnsafeCell; and the rest of the reference returned by self.data() is unused and hence no problem.