Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for raw stabilization (raw::TraitObject) #27751

Closed
aturon opened this issue Aug 12, 2015 · 36 comments · Fixed by #86833
Closed

Tracking issue for raw stabilization (raw::TraitObject) #27751

aturon opened this issue Aug 12, 2015 · 36 comments · Fixed by #86833
Labels
A-trait-system Area: Trait system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Aug 12, 2015

The std::raw module exports some representation details for the purposes of transmuting and taking things apart. Stabilizing these means pinning down the representation details forever.

One possibility is to instead provide various conversions, rather than asking people to use transmute, when we're sure the conversions will always be possible. Methods like from_raw_parts are already doing this.

The TraitObject case in particular is unsettled due to the potential for upcasting in the future.

cc @carllerche

CURRENT STATUS AS OF 2016-08-18: Discussed in @rust-lang/lang meeting. The situation is this. We expect that this will be insufficient in the future, particularly if/when we support trait objects like Trait1+Trait2 (which would require 2 vtables). We could stabilize it, since it would remain correct for all existing objects, but it'd be a shame. In contrast, the various "custom DST" proposals would allow for a much more flexible way of extracting and creating this data (sort of like from_raw_parts), so we think we'd prefer to explore that route. (from #27751 (comment))

@aturon aturon added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@SimonSapin
Copy link
Contributor

raw::Slice can be replaced with [T]::len, [T]::as{,_mut}_ptr, and slice::from_raw_parts{,_mut}. Kill it.

TraitObject is used in Servo:

  • To implement downcasting from trait objects of a custom trait, like std::error::Error does. It’s unfortunate that std::any::Any can not be re-used. (I understood why at some point, but forgot.)
  • Using obj.data as usize as an identifier. (LayoutId, OpaqueFlow)
  • To do some kind of inheritance. Getting a reference from a &Flow trait object to the BaseFlow (concrete struct) that is must be the first field of every implementors of the Flow trait. Yes, relying on struct field order.
  • To emulate Arc<Trait> before Arc supported DSTs. I’m trying to switch to Arc now.

@carllerche
Copy link
Member

I have used TraitObject a number of times.

One case, in the bytes crate, I use TraitObject to avoid an allocation, which can add up to a significant performance win in some cases (for example, when using Rope).

The insight is that, pretty often, implementations of ByteStr can fit in a pointer size. So, Bytes, which is a wrapper around a ByteStr trait object does an optimization where the data component of the trait object gets "inlined".

Another case I had was building a somewhat involved tree structure, there would be many pointers to the same trait object. The goal was to get an entire node of the tree to fit in a cache line and duplicating the vtable ptr prevented this. So, in this case, I saved the vtable pointer only once and "reconstructed" the trait object on demand.

bors added a commit that referenced this issue Aug 18, 2015
also, use the right caching logic for type_moves_by_default (this was
broken by @jroesch).

```
before:
593.10user 5.21system 7:51.41elapsed 126%CPU (0avgtext+0avgdata 1150016maxresident)k

after:
567.03user 4.00system 7:28.23elapsed 127%CPU (0avgtext+0avgdata 1133112maxresident)k
```

A nice 4.5% improvement. For reference, on the last run LLVM takes 429.267s, which is 75% - hopefully this can be reduced.

I think the regression since #27751 is because of the wf patch - need to investigate it through.

r? @nikomatsakis
@kamalmarhubi
Copy link
Contributor

I have a possible use case for stabilizing Slice: I would like to use byte slices directly as struct iovec for readv and writev in nix. There's an issue with more detail: nix-rust/nix#263

Mostly I'm looking to exploit the (pointer, length) representation matching up with iovec, and thereby allowing a nice ergonomics win for calls using iovecs.

@alexcrichton
Copy link
Member

🔔 This issue is now entering its cycle-long final comment period 🔔

Specifically, the libs team is considering deprecating raw::Slice and raw::Repr as these are replaced by std::slice functions.

@kamalmarhubi we specifically don't want to stabilize the layout of slices to be used in places like that, hence the deprecation!

@alexcrichton alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Mar 11, 2016
@SimonSapin
Copy link
Contributor

Was TraitObject discussed?

@alexcrichton
Copy link
Member

Yes, and we neither want to stabilize it nor deprecate it yet, this module will stick around for that reason.

@aturon
Copy link
Member Author

aturon commented Mar 11, 2016

A bit more detail from the discussion at the meeting: TraitObject is, amongst the types that have ever appeared in this module, the most likely to change in the future. That's in part due to considerations about upcasting, multiple trait inheritance, and so on. It's unclear whether we will ever want to commit firmly to a representation here.

That said, it'd be good to use this issue to track usecases for the raw representation, and see whether we can find other ways of accommodating them.

@kamalmarhubi
Copy link
Contributor

@alexcrichton

@kamalmarhubi we specifically don't want to stabilize the layout of slices to be used in places like that, hence the deprecation!

I figured as much. Maybe specialization and conditional compilation will let me be bad in the way I want to be there... :-)

Thanks for the ping!

@alexcrichton
Copy link
Member

The libs team discussed this during triage yesterday and the decision was to deprecate

@kamalmarhubi
Copy link
Contributor

@alexcrichton is that the whole module, or just Slice and Repr as mentioned above:

Specifically, the libs team is considering deprecating raw::Slice and raw::Repr as these are replaced by std::slice functions.

@nrc
Copy link
Member

nrc commented Apr 7, 2016

reluctant +1 - I kind of liked the raw::Slice approach, but the std::slice solutions do the job just as well and we only need one.

@alexcrichton
Copy link
Member

@kamalmarhubi just the Slice and Repr traits, as noted in #32804

@SimonSapin
Copy link
Contributor

It looks like fat raw pointers can be cast to thin raw pointers to access the "data" part: (x: &Trait) as *const Trait as *const (). This leaves missing:

  • Some way to extract a vtable pointer from &Trait or *const Trait. (I don’t know if it is possible or desirable to exclude other kinds of dynamically-sized types from this. Would a Trait trait make sense? For use like &T where T: Trait.)
  • Some unsafe from_raw_parts function or method that returns *const Trait or *mut Trait (which can then be used with &mut *x or Box::from_raw(x)) from separate data and vtable pointers.

These together could replace std::raw::TraitObject without forcing users to make memory representation assumptions e.g. by transmuting *const Trait to and from (*const (), *const ()).

@SimonSapin
Copy link
Contributor

I don’t know if this was always the case, but I’ve just found out that casting a raw pointer from a ?Sized type to a Sized one is allowed, and for trait objects extracts the data pointer. I’ve replaced with this all uses of std::raw::TraitObject in Servo.

fn data_pointer(object: &SomeTrait) -> *const () {
    let ptr: *const SomeTrait = object;
    ptr as *const ()
}

@eddyb
Copy link
Member

eddyb commented Oct 13, 2017

@SimonSapin You're also able to make that function generic over a T: ?Sized pointee, right?

EDIT: I mean this:

fn data_pointer<T: ?Sized>(r: &T) -> *const u8 {
    r as *const T as *const u8
}

@SimonSapin
Copy link
Contributor

@eddyb Yes, correct. In fact Servo does this, with a trait bound, to allow both trait objects and concrete types that implement that trait.

@durka
Copy link
Contributor

durka commented Oct 13, 2017 via email

@SimonSapin
Copy link
Contributor

What do you mean, which thin part? I don’t think it would make sense for this cast to extract a vtable pointer or a slice length when casting to an arbitrary pointer to a statically-sized type.

@durka
Copy link
Contributor

durka commented Oct 13, 2017 via email

@SimonSapin
Copy link
Contributor

Even if the repr changes it has to include at least a data pointer, doesn’t it?

@durka
Copy link
Contributor

durka commented Oct 13, 2017

The reference says it is a "pointer-to-pointer cast" but doesn't say what that means. Sounds OK though?

@eddyb
Copy link
Member

eddyb commented Oct 13, 2017

@durka The casts are properly defined and there's even fat-to-fat casts which check the metadata and whatnot - they might be described in an RFC, or not at all, but that's orthogonal.

@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 13, 2017

https://doc.rust-lang.org/nightly/nomicon/casts.html says:

*T as *U where T, U: Sized
*T as *U TODO: explain unsized situation

and

Note that lengths are not adjusted when casting raw slices - *const [u16] as *const [u8] creates a slice that only includes half of the original memory.

Maybe it’s time to fix that TODO and define this more precisely?

@SimonSapin
Copy link
Contributor

rust-lang/rfcs#2580 proposes deprecating this.

@SimonSapin
Copy link
Contributor

Deprecation PR: #84207

@SimonSapin
Copy link
Contributor

As an unstable feature, this can be removed a couple release cycles after the deprecation warning has been in place.

@crlf0710
Copy link
Member

crlf0710 commented Jul 3, 2021

Opened a removal pr at #86833.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC Libs-Tracked Libs issues that are tracked on the team's project board. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.