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] Add a `Resource.access_mut` method #24

Closed
japaric opened this Issue May 10, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@japaric
Owner

japaric commented May 10, 2017

Problem

Currently, when you want to share some data structure that needs to be mutated through a mutable (&mut-) reference between tasks you have no option but to wrap it in a RefCell because the Resource.access method only returns shared (&-) references. This involves runtime overhead because the RefCell does runtime checks to ensure that accessing the inner data preserves Rust's borrowing semantics (one &mut- reference OR several &- references)

static R1: Resource<RefCell<Thing>, C1> = Resource::new(RefCell::new(Thing::new()));

fn t1(_: Exti0, ref prio: P1, ref thr: T1) {
    let r1 = R1.access(prio, thr);
    r1.borrow().foo(); // runtime check
    r1.borrow_mut().foo_mut(); // runtime check
}

Due to how RTFM works it will be always be the case that a resource that contains a RefCell will have no outstanding borrows when it's first accessed in a task. This means that provided that you don't break borrow semantics within a task then executing the task will never fail a runtime borrow check. The compiler doesn't know this though, and will still perform the borrow checks at runtime even if they are effectively infallible.

Workaround (?)

To remove the runtime checks the assume intrinsic can be used to tell the compiler about the "no outstanding borrow on first access" property:

static R1: Resource<RefCell<Thing>, C1> = Resource::new(RefCell::new(Thing::new()));

fn t1(_: Exti0, ref prio: P1, ref thr: T1) {
    let r1 = R1.access(prio, thr);
    unsafe { intrinsics::assume(r1.borrow_state() == BorrowState::Unused) }
    r1.borrow().foo(); // no runtime check
    r1.borrow_mut().foo_mut(); // no runtime check
}

With this information the compiler is able to optimize away the runtime checks. The downside is that this requires unsafe, borrow_state is a deprecated API and that this doesn't optimize away the RefCell reference counter from the Resource<RefCell<_>> memory representation.

Possible solution

A possible solution is to add an access_mut method to Resource that's similar to Local.borrow_mut and that has the following signature:

// trait bounds not shown for brevity
impl<T, RC> Resource<T, RC> {
    // it must hold that: TP <= RC <= PT
    fn access_mut(&'static self, priority: &mut TP, threshold: &PT) -> &mut T {
        // ..
    }
}

The problem here are the lifetime constraints. The borrow &mut T can't outlive the threshold token, or it would be possible for it to outlive a Threshold.raise critical section and that would lead to data races. Additionally, the borrow &mut T must also freeze the priority token; this is required to avoid mutable aliasing -- without this it would be possible to get two or more mutable references to the data protected by the Resource abstraction. Alternatively, this method could request a mutable reference to the task token to prevent aliasing but the priority token will still be required (as a shared reference) so the signature would require 3 tokens.

Whatever we end up doing these operations must result in compile errors:

{
    // reference to inner data escaped the critical section
    let bad_r1 = threshold.raise(&R1, |threshold| {
        R1.access_mut(..)
    });
}
let r1: &mut Foo = R1.access_mut(..);
let r1_alias: &mut Foo = R1.access_mut(..);

This solution suffers from the problem that it's overly restrictive when it comes to borrowing different resources. You can't take a mutable reference to resource A and within the same scope take a shared reference to resource B (where A != B). Local has the same problem so let's use it to illustrate the problem:

fn t1(mut task: Exti0, ref prio: P1, ref thr: T1) {
    static A: Local<(), Exti0> = Local::new(());
    static B: Local<(), Exti0> = Local::new(());

    {
        // This is perfectly legal but the API prevents it
        let a = A.borrow(&task);
        let b = B.borrow_mut(&mut task);
    }

    {
        // what the API really wants to prevent is this
        let a = A.borrow(&task);
        let a_alias = A.borrow_mut(&mut task);
    }
}

The workaround for this borrow checker problem is to minimize the borrows to free up the task token ASAP. So something like this:

fn t1(mut task: Exti0, ref prio: P1, ref thr: T1) {
    static A: Local<(), Exti0> = Local::new(());
    static B: Local<(), Exti0> = Local::new(());

    A.borrow(&task).foo();
    B.borrow_mut(&mut task).foo_mut();
}

But this very unergonomic and doesn't work if you have a function call that must be called with a shared reference to resource A and a mutable reference to resource B:

fn t1(mut task: Exti0, ref prio: P1, ref thr: T1) {
    static A: Local<(), Exti0> = Local::new(());
    static B: Local<(), Exti0> = Local::new(());

    bar(A.borrow(&task), B.borrow_mut(&mut task));
    //~^ error
}

Ideal solution

The ideal solution should allow mutably borrowing resources without requiring runtime checks and without preventing borrowing other resources. In my mind such solution would involve some sort of compile time task local borrow checker which I have no idea of how to implement but that would probably require making every single resource have a different type (static A: Resource<(), C1> and static B: Resource<(), C1> would need to have different types). I expect the implementation to be rather unergonomic as well, unless we implement it as a compiler plugin of sorts.

@japaric

This comment has been minimized.

Show comment
Hide comment
@japaric

japaric Jul 29, 2017

Owner

The main issues has been fixed in #34

Owner

japaric commented Jul 29, 2017

The main issues has been fixed in #34

@japaric japaric closed this Jul 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment