crossbeam-rs / rfcs

RFCs for changes to Crossbeam
Apache License 2.0
147 stars 14 forks source link

Add the generalized-scope RFC #10

Closed jeehoonkang closed 7 years ago

jeehoonkang commented 7 years ago

I am proposing:

This RFC is fully implemented in this branch.

Rendered

ghost commented 7 years ago

Reviewing only the changes related to unprotected now, and will look into realms later.

First of all, I think it's reasonable to optimize code whenever we can. But in this case I'm wondering if this is the right direction to push optimizations into.

Why would someone want to call defer{,_free,_drop} in an unprotected scope? I would argue that, if you're calling defer inside an unprotected scope, there's a good chance you might be doing something wrong, or inadvertently writing code that does something you don't want to do. It's just such a fishy thing to do. :)

Is the intention to immediately destroy the Ptr? After all, manually doing drop(Box::from_raw(p.as_raw() as *mut Node<>T)) is much uglier than scope.defer_drop(p), so I can see the appeal behind that.

If that is indeed the case, perhaps we'd like an unsafe method Ptr::destroy that drops and deallocates the object. Then you could simply do p.destroy(). And to follow up a bit, perhaps we could then rename defer_drop to defer_destroy and implement it like this:

impl<'scope> Scope<'scope> {
    unsafe fn defer_destroy<T: 'static>(&self, ptr: Ptr<'scope, T>) {
        let ptr: Ptr<'static, T> = mem::transmute(ptr);
        self.defer(move || ptr.destroy());
    }

    // ...
}
jeehoonkang commented 7 years ago

@stjepang thank you for the feedback! I would like to answer some of your questions.

On defer* methods in unprotected scopes, I initially thought the same thing, but in implementing it in crossbeam-epoch I met the following code snippet: https://github.com/crossbeam-rs/crossbeam-epoch/blob/master/src/sync/queue.rs#L184 Here, a queue's drop method repeatedly try_pop() its elements in an unprotected scope. Which is justified because the calling thread is the only one accessing the queue. However, inside try_pop(), a node of the queue is defer_drop()ped. That's the primary reason I implemented defer_* for unprotected scopes.

Unprotected scopes do not need a garbage bag. So using the is_protected boolean field in Scope instead of using UnprotectedScope will incur performance cost, which is probably small, though.

I very much like the idea of Ptr::destroy(). If there is no significant regressions, I would like to go for it.

ghost commented 7 years ago

I initially thought the same thing, but in implementing it in crossbeam-epoch I met the following code snippet

I see -- this is indeed a good case for calling defer* inside an unprotected scope.

Unprotected scopes do not need a garbage bag. So using the is_protected boolean field in Scope instead of using UnprotectedScope will incur performance cost, which is probably small, though.

Actually, we don't need an additional boolean field. Here we should just check whether self.bag is null. If it is null, we destroy garbage immediately, otherwise we push it into the bag.

This is a very easy solution, and now there's no need to generalize scopes. :) The only thing left to do is to clarify in the documentation what defer* does in unprotected scopes.

jeehoonkang commented 7 years ago

I'm closing it. In #12, we are discussing what will be the right interface for the "realms", which might evolve to an RFC later.