Yeet `owning_ref` #109971

WaffleLapkin · 2023-04-05T13:54:43Z

Based on the discussions from #109948

This replaces owning_ref with a far simpler & safer abstraction.

Turns out - `owning_ref` is unsound due to `Box` aliasing stuff - `rustc` doesn't need 99% of the `owning_ref` API - `rustc` can use a far simpler abstraction that is `OwnedSlice`

rustbot · 2023-04-05T13:54:49Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

petrochenkov · 2023-04-05T14:55:08Z

Could someone summarize why this type is necessary?
If you have an owned Box or Mmap, why do you need to additionally pack a reference with it?

Noratrieb · 2023-04-05T15:21:37Z

Metadata handling stores references into subsections of the object file. As we've seen in #97770, metadata handling is very perf sensitive so just storing indices doesn't seem to work.

@bors try @rust-timer queue

Noratrieb · 2023-04-05T15:26:18Z

compiler/rustc_data_structures/src/owned_slice.rs

+pub fn try_slice_owned<O, F, E>(owner: O, slicer: F) -> Result<OwnedSlice, E>
+where
+    O: Send + Sync + 'static,
+    F: Fn(&O) -> Result<&[u8], E>,


FnOnce, this avoids having to change it in other places and is simply more correct.

Noratrieb

It's a lovingly simple datastructure, but even lovingly simple datastructures deserve tests 😅

cramertj · 2023-04-05T18:28:45Z

compiler/rustc_codegen_ssa/src/back/metadata.rs

+
+    unsafe { Mmap::map(file) }
+        .map_err(|e| format!("failed to mmap file '{}': {}", path.display(), e))
+        .and_then(|mmap| try_slice_owned(mmap, |mmap| f(mmap)))


|mmap| f(mmap)

nit: Can this be replaced with f?

not with &f either?

noamtashma · 2023-04-06T03:12:58Z

This isn't necessarily relevant to yeeting owning_ref, but this issue caused me to take a look at the nested Box<dyn {Lrc<Box<dyn {?}>}> structure in def_path_hash_map and I think it could be improved further.

It seems that the metadata is only actually used in the form of MetadataRef, so that it's cloneable. If that's true, than we could instead always use the structure of Lrc<dyn Send + Sync>.

In other words, have the owning_slice contain an Lrc<dyn Send + Sync> instead of a Box<dyn Send + Sync>.
Then owning_slice becomes clone, it gains a slice method or equivalent to get a subslice of the existing slice without an extra indirection, and MetadataRef and MetadataBlob become equivalent, which is nice.

Any opinions?

petrochenkov · 2023-04-06T07:51:02Z

r? @Nilstrieb

Noratrieb · 2023-04-06T12:06:27Z

try was cancelled by the push
@bors try @rust-timer queue

bors · 2023-04-06T12:06:37Z

⌛ Trying commit 504c4c4 with merge 8acf625821137194e53ab4dc44492235feb70809...

WaffleLapkin · 2023-04-06T12:46:51Z

@noamtashma this is certainly possible, but I'm not sure if that actually changes anything, because OwningSlice accesses don't get slower with more indirection (it always stores the pointer to actual data).

I'd prefer to keep this PR as-is and then try your idea in a follow-up, just so it's easier to review / keep track of.

bors · 2023-04-06T13:50:07Z

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

Noratrieb · 2023-04-06T19:04:06Z

Yeet!
@bors r+ rollup=maybe

bors · 2023-04-06T19:04:08Z

📌 Commit 2733c29 has been approved by Nilstrieb

It is now in the queue for this repository.

klensy · 2023-04-06T20:01:15Z

compiler/rustc_data_structures/src/owned_slice.rs

+}
+
+impl Borrow<[u8]> for OwnedSlice {
+    fn borrow(&self) -> &[u8] {


It will be nice to check, that this (and other short fns around) call will be inlined, as rust sometimes forget about that.

Great call, these two functions should all have #[inline] attributes. I'm not entirely sure where they are all used but given that it's used in metadata loading which is very hot this could be really bad on platforms that aren't compiled with LTO (like our neutral perf run was).

Noratrieb · 2023-04-06T20:03:47Z

@bors r-

WaffleLapkin · 2023-04-07T10:19:00Z

@bors r=Nilstrieb rollup=maybe

bors · 2023-04-07T10:19:02Z

📌 Commit fbe0591 has been approved by Nilstrieb

It is now in the queue for this repository.

…strieb Yeet `owning_ref` Based on the discussions from rust-lang#109948 This replaces `owning_ref` with a far simpler & safer abstraction. Fixes rust-lang#109974

bors · 2023-04-08T01:08:29Z

⌛ Testing commit fbe0591 with merge 0f0dc29...

bors · 2023-04-08T03:26:48Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 0f0dc29 to master...

bors · 2023-04-08T03:26:48Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 0f0dc29 to master...

rust-timer · 2023-04-08T05:18:19Z

Finished benchmarking commit (0f0dc29): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

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

Cycles

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

WaffleLapkin added 4 commits April 5, 2023 13:49

Implement OwnedSlice

689beda

Use OwnedSlice instead of owning_ref

c0ceefd

Yeet owning_ref

d705654

Turns out - `owning_ref` is unsound due to `Box` aliasing stuff - `rustc` doesn't need 99% of the `owning_ref` API - `rustc` can use a far simpler abstraction that is `OwnedSlice`

Fix typo

9405f58

rustbot assigned petrochenkov Apr 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2023

WaffleLapkin mentioned this pull request Apr 5, 2023

Small owning ref refactoring #109948

Closed

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2023

Noratrieb reviewed Apr 5, 2023

View reviewed changes

Noratrieb requested changes Apr 5, 2023

View reviewed changes

Noratrieb mentioned this pull request Apr 5, 2023

OwningRef is included in the compiler despite known soundness problems. #109974

Closed

Add context to phantom comment

504c4c4

Noratrieb force-pushed the yeet_ownership branch from e1af75e to 504c4c4 Compare April 5, 2023 16:56

cramertj reviewed Apr 5, 2023

View reviewed changes

rustbot assigned Noratrieb and unassigned petrochenkov Apr 6, 2023

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the yeet_ownership branch from e06f470 to 2733c29 Compare April 6, 2023 18:13

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2023

klensy reviewed Apr 6, 2023

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 6, 2023

Mark OwnedSlice::{deref, borrow} as #[inline]

fbe0591

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2023

This was referenced Apr 7, 2023

Rollup of 5 pull requests #110053

Closed

Rollup of 5 pull requests #110054

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2023

bors merged commit 0f0dc29 into rust-lang:master Apr 8, 2023

rustbot added this to the 1.70.0 milestone Apr 8, 2023

WaffleLapkin deleted the yeet_ownership branch April 10, 2023 19:26

ehuss mentioned this pull request Apr 14, 2023

Spurious CI failure: tests/run-make/jobserver-error #110321

Open

tshepang mentioned this pull request Jul 2, 2023

OwningRef exists no more rust-lang/rustc-dev-guide#1715

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yeet `owning_ref` #109971

Yeet `owning_ref` #109971

WaffleLapkin commented Apr 5, 2023 •

edited by Noratrieb

Loading

rustbot commented Apr 5, 2023

petrochenkov commented Apr 5, 2023

Noratrieb commented Apr 5, 2023

This comment has been minimized.

Noratrieb Apr 5, 2023

Noratrieb left a comment

cramertj Apr 5, 2023

WaffleLapkin Apr 5, 2023

kadiwa4 Apr 6, 2023

WaffleLapkin Apr 6, 2023

noamtashma commented Apr 6, 2023

petrochenkov commented Apr 6, 2023

Noratrieb commented Apr 6, 2023

This comment has been minimized.

bors commented Apr 6, 2023

WaffleLapkin commented Apr 6, 2023

bors commented Apr 6, 2023

This comment has been minimized.

Noratrieb commented Apr 6, 2023

bors commented Apr 6, 2023

klensy Apr 6, 2023

Noratrieb Apr 6, 2023

Noratrieb commented Apr 6, 2023

WaffleLapkin commented Apr 7, 2023

bors commented Apr 7, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

Yeet owning_ref #109971

Yeet owning_ref #109971

Conversation

WaffleLapkin commented Apr 5, 2023 • edited by Noratrieb Loading

rustbot commented Apr 5, 2023

petrochenkov commented Apr 5, 2023

Noratrieb commented Apr 5, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Noratrieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamtashma commented Apr 6, 2023

petrochenkov commented Apr 6, 2023

Noratrieb commented Apr 6, 2023

This comment has been minimized.

bors commented Apr 6, 2023

WaffleLapkin commented Apr 6, 2023

bors commented Apr 6, 2023

This comment has been minimized.

Noratrieb commented Apr 6, 2023

bors commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Apr 6, 2023

WaffleLapkin commented Apr 7, 2023

bors commented Apr 7, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

Overall result: no relevant changes - no action needed

Yeet `owning_ref` #109971

Yeet `owning_ref` #109971

WaffleLapkin commented Apr 5, 2023 •

edited by Noratrieb

Loading