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

RFC for placement box with Placer trait for overloading. #470

Closed
wants to merge 2 commits into from

Conversation

pnkfelix
Copy link
Member

Summary

Add user-defined placement in expression (more succinctly, "an in expression"), an operator analogous to "placement new" in C++. This provides a way for a user to specify (1.) how the backing storage for some datum should be allocated, (2.) that the allocation should be ordered before the evaluation of the datum, and (3.) that the datum should preferably be stored directly into the backing storage (rather than allocating temporary storage on the stack and then copying the datum from the stack into the backing storage).

This RFC does not suggest any change to the non-placement box expression special form (box <value-expr>); a future RFC is planned that will suggest changes to that form based on preliminary feedback to this RFC, but that is orthogonal to this RFC's goals.

rendered

@bill-myers
Copy link

I'm not sure placement box is in general a good design.

The big issue is that the only thing that it solves is elimination of a temporary, but only in a specific case and in a way that does not easily compose.

For example, let's say that you want to create a mechanism to box T into a Box<Option> by using Some or a mechanism to box two values in a pair.

The natural composable way to do so is this:

fn box_as_option(t: T) -> Box<Option<T>> {
   box Some(t)
}

fn box_as_pair(t: T, u: U) -> Box<(T, U)> {
   box (t, u)
}

The problem is that this results in creating temporaries again, even if we have placement box... and the only way to avoid this is an extremely ugly hack with unsafe code that implements the placement box traits to do this simple construction operation.

Also it seems it's completely impossible to box more than one object (for instance, by boxing as a pair) without temporaries even with hacks in this RFC. This RFC also provides an example of "emplace back" that takes 54 lines of code (!) for something that should take 1 line.

IMHO any design, such as the one in this RFC, that doesn't allow such composability easily should probably be avoided.

I think the simplest solution is to just remove the box keyword, use Box::new and rely on inlining and the optimizer to remove the temporaries, with an annotation on heap allocation functions that tells LLVM that constant propagation, move elimination and similar can be done across heap allocation.

@pnkfelix
Copy link
Member Author

@bill-myers you make good points, but I can do without hyperbole like this:

"emplace back" that takes 54 lines of code (!) for something that should take 1 line.

A lot of the code in that example can be removed and is only there for expository purposes (or for testing in my prototype). I do (and did) acknowledge that the code there is overcomplicated by the protocol's use of two separate traits; see discussion in alternatives section


Having said that, the composability problem is an interesting one. I'm not sure a solution exists that is (1.) safe, (2.) composable in the way you outline, and (3.) guaranteed to avoid a temporary. This is not just about the stdlib's heap allocation functions; it is also about user-defined memory abstractions, like arenas.

But maybe you are suggesting that even the latter should receive the annotations to convince LLVM that it is safe to do optimizations across their procedure call boundary?

@pnkfelix
Copy link
Member Author

I just remembered: @nikomatsakis told me a while back that a number of the types I wrote as type parameters should instead be associated types. I will try to update the RFC accordingly tomorrow or Thursday.

@huonw
Copy link
Member

huonw commented Nov 18, 2014

I think the simplest solution is to just remove the box keyword, use Box::new and rely on inlining and the optimizer to remove the temporaries, with an annotation on heap allocation functions that tells LLVM that constant propagation, move elimination and similar can be done across heap allocation.

Relying on the implementation details of the optimiser seems unfortunate. Also, we would need to make it happen even without optimisations (otherwise, say, Box::new(ten_megabyte_array) will never work in non-release mode) and, we would also probably want to provide some explicit way to allow the programmar to guarantee the behaviour manually.

@reem
Copy link

reem commented Nov 19, 2014

A lot of the apparent complexity in this RFC appears to come from a lack of a safe way to talk about "out" pointers, or pointers that are not initialized and are meant to be initialized before use, "move" pointers that take ownership of the thing they are referencing without moving or boxing it, and the lack of HKT. This whole thing could easily be written as just:

pub trait Placer {
   type Place<*>; // Theoretical HKT syntax for a * -> * type.
   fn place<T>(&mut self) -> &out T;
   fn finalize<T>(&mut self, &move T) -> Place<T>;
}

(or something similar) if we had &out and &move references and HKT. The lack of these things is completely understandable given the 1.0 timeframe, but committing to a complicated and inherently unsafe design in the meantime means that we will not be able to simplify things until 2.0, which would be a shame.

Here's a sample implementation of Box and Vec::emplace_back using the above:

// Box
pub struct BoxPlacer;

impl Placer for BoxPlacer {
    type Place = Box;
    fn place<T>(&mut self) -> &out T {
        let size = mem::size_of::<T>();
        let align = mem::align_of::<T>();

        if size == 0 {
            (heap::EMPTY as *mut u8).into_out()
        } else {
            let p = unsafe {
                // into_out does a NULL check
                // this implementation could be made *safe* if there was a heap::allocate_out
                heap::allocate(size, align).into_out()
            };
        }
    }

    fn finalize<T>(&mut self, val: &move T) -> Box<T> {
        unsafe { mem::transmute::<&move T, Box<T>>(val) }
    }
}

// And emplace back
pub struct Emplacer(&mut Vec<()>);

impl<T> Placer for Emplacer<T> {
   type Place = // some standard `* -> ()` HKT goes here
   fn place<T>(&mut self) -> &out T {
       // Grows the Vec if needed and gets an &out reference to a spot past the end, implementation detail.
       self.0.place_back()
   }

   fn finalize<T>(&mut self) {}
}

For the record, I have clearly not given this nearly as much thought as @pnkfelix has in this RFC, I just think it's worth questioning whether future language features could vastly simplify the situation here, and, if they could, whether it's worth adding this feature now or just reserving the necessary syntax.

@pnkfelix
Copy link
Member Author

@reem ah I think your comment is a much much more coherent description of what I was thinking of when discussing &uninit in the unresolved questions section

@glaebhoerl
Copy link
Contributor

See also the ideas in #413 which seems vaguely similar to what @reem wrote. (I haven't had the time/capacity to read and digest the RFC yet... not sure when I will.)

@reem
Copy link

reem commented Nov 19, 2014

I think the major hole in the implementation I proposed above is that it doesn't handle exceptions while evaluating <value-expr>, since it only allows returning &out T. Instead, it would have to return a special handle type that could take care of cleanup and also has a method which gives out an &out T.

Something like:

pub trait OutHandle<T> {
   fn out(self) -> &out T;
}

@vadimcn
Copy link
Contributor

vadimcn commented Nov 20, 2014

@bill-myers, can you please expand on why tuple placement is not possible without allocating temporaries?
Let's say that U is large enough that it cannot be returned in registers. Then it must be returned via a pointer to caller-allocated memory. If we've allocated a place for (T,U), can we simply give U's constructor a pointer to the U part of the tuple?

@Ericson2314
Copy link
Contributor

I completely agree with @reem regarding &out. I'm not sure what should be done in the short term, but it should be kept unstable. Long term, I think we should implement &out and friends, and then try to come up with a syntax. &out takes care of safety, allowing us to focus on ergonomics separately.

(As an aside, the current return value optimization also kinda creeps me out (despite my background functional programming), and I wonder if something a bit more explicit but still ergonomic could be done instead.)

Having said that, the composability problem is an interesting one. I'm not sure a solution exists that is (1.) safe, (2.) composable in the way you outline, and (3.) guaranteed to avoid a temporary. This is not just about the stdlib's heap allocation functions; it is also about user-defined memory abstractions, like arenas. -- @pnkfelix

Once-closure that takes a &out I believe satisfies all three. Here is a first attempt at making it ergonomic:

At the rust meeting August 2014, the team thought we could do a simpler desugaring that would wrap the in a once-function closure; this way, you would still force the expected order-of-evaluation (do the allocation first, then run the closure, letting return value optimization handle writing the result into the backing storage).

The most obvious place where this completely falls down is that it does not
do the right thing for something like this:

let b: PBox<T> = box (place) try!(run_code())

because under the once-function desugaring, that gets turned into something
like:

place.make_box(|once: | -> T { try!(run_code()) })

-- @pnkfelix

The double try is a bit unfortunate, but what if :

let b: PBox<T> = try!(box (place) try!(run_code()));

becomes:

let b: PBox<T> = try!(place.make_box::<_>(|once: ptr: &out T| -> R<T> {
  ptr = try!(run_code());
  // Just need some way to add the Ok(())
},));

Granted, I punt on the Ok(()), but I view error-agnostic higher-order functions as a separate problem.

@nikomatsakis
Copy link
Contributor

So conversation here has slowed down, but I want to add my current thoughts. Let's leave aside the particulars of the protocol for the moment. The high-level question is this: for 1.0, we want to have an in-place allocation / construction syntax, or if we want to start without one and bring one in later.

The argument in favor of adding the syntax now is probably two-fold. First, there is a consistency argument. If we were to remove this syntax, it would mean that (for the time being) the box key would be feature-guarded, and hence you would build boxes like Box::new(...). This then implies that, in the future, when the box/in keywords are added, there would be two correct ways to construct boxes / RCs / etc (or at least one way that is older and very common). Second, there is likely some performance cost due to increased temporaries, which has not been measured.

The argument against adding the syntax is that it is clearly not necessary for 1.0 and we may be able to concoct a better protocol in the future. It would also allow us to introduce the box keyword in tandem across the board -- construction and patterns. Finally it would help to free up developer time to focus on higher priority things for 1.0, although Felix has already done a lot o the implementation work here, so it's not clear how useful that is (but there still remains a non-trivial amount of work to be done, in any case).

We had this discussion in a recent meeting (last Tuesday) and the general feeling was to lean towards removing box from the 1.0 release. That said, it's important to recognize that this too requires some effort (first, measurement to see its impact, but also just editing the existing code).

@reem
Copy link

reem commented Dec 15, 2014

@nikomatsakis Why could we not maintain the status quo and simply special-case box for now, leading to backwards-compatible extensions in the future? Removing box (or feature-gating it) would be a real ergonomics hit, I think.

I am for waiting for post-1.0 to implement extensible box, but we could still have non-extensible box.

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 4, 2015

closing in favor of the more conservative #809

@pnkfelix pnkfelix closed this Feb 4, 2015
bors added a commit to rust-lang/rust that referenced this pull request Apr 4, 2018
…sakis

Remove all unstable placement features

Closes #22181, #27779. Effectively makes the assortment of placement RFCs (rust-lang/rfcs#470, rust-lang/rfcs#809, rust-lang/rfcs#1228) 'unaccepted'. It leaves `box_syntax` and keeps the `<-` token as recognised by libsyntax.

------------------------

I don't know the correct process for unaccepting an unstable feature that was accepted as an RFC so...here's a PR.

Let me preface this by saying I'm not particularly happy about doing this (I know it'll be unpopular), but I think it's the most honest expression of how things stand today. I've been motivated by a [post on reddit](https://www.reddit.com/r/rust/comments/7wrqk2/when_will_box_and_placementin_syntax_be_stable/) which asks when these features will be stable - the features have received little RFC-style design work since the end of 2015 (~2 years ago) and leaving them in limbo confuses people who want to know where they're up to. Without additional design work that needs to happen (see the collection of unresolved questions later in this post) they can't really get stabilised, and I think that design work would be most suited to an RFC rather than (currently mostly unused) experimental features in Rust nightly.

I have my own motivations - it's very simple to 'defeat' placement in debug mode today and I don't want a placement in Rust that a) has no guarantees to work and b) has no plan for in-place serde deserialisation.

There's a quote in [1]: "Ordinarily these uncertainties might lead to the RFC being postponed. [The RFC seems like a promising direction hence we will accept since it] will thus give us immediate experience with the design and help in determining the best final solution.". I propose that there have been enough additional uncertainties raised since then that the original direction is less promising and we should be think about the problem anew.

(a historical note: the first mention of placement (under that name - uninit pointers were earlier) in an RFC AFAIK is [0] in late 2014 (pre-1.0). RFCs since then have built on this base - [1] is a comment in Feb 2015 accepting a more conservative design of the Place* traits - this is back when serde still required aster and seemed to break every other nightly! A lot has changed since then, perhaps placement should too)

------------------------

Concrete unresolved questions include:

 - making placement work in debug mode [7]
 - making placement work for serde/with fallible creation [5], [irlo2], [8]
 - trait design:
   - opting into not consuming the placer in `Placer::make_place` - [2]
   - trait proliferation - [4] (+ others in that thread)
   - fallible allocation - [3], [4] (+ others in that thread)
 - support for DSTs/unsized structs (if at all) - [1], [6]

More speculative unresolved questions include:

 - better trait design with in the context of future language features [irlo1] (Q11), [irlo3]
 - interaction between custom allocators and placement [irlo3]

[0] rust-lang/rfcs#470
[1] rust-lang/rfcs#809 (comment)
[2] rust-lang/rfcs#1286
[3] rust-lang/rfcs#1315
[4] #27779 (comment)
[5] #27779 (comment)
[6] #27779 (comment)
[7] #27779 (comment)
[8] rust-lang/rfcs#1228 (comment)
[irlo1] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789
[irlo2] https://internals.rust-lang.org/t/placement-nwbi-faq-new-box-in-left-arrow/2789/19
[irlo3] https://internals.rust-lang.org/t/lang-team-minutes-feature-status-report-placement-in-and-box/4646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants