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

Add the generalized-scope RFC #10

Closed

Conversation

jeehoonkang
Copy link
Contributor

@jeehoonkang jeehoonkang commented Sep 4, 2017

I am proposing:

  • making Scope trait, and implements Scope for the old Scope struct.
  • letting unprotected use a different implementation of Scope so that defer_dropped and defer_freed objects are immediately dropped and freed.
  • supporting multiple EBR instances.

This RFC is fully implemented in this branch.

Rendered


# Motivation

The `unprotected` function provides a `Scope` without pinning the thread, where the caller thread is
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More precisely (see the Atomic API RFC), the caller thread is either the only one accessing atomics or no one is modifying atomics. This is sort of like a RwLock.

The `unprotected` function provides a `Scope` without pinning the thread, where the caller thread is
the only one accessing atomics. Assuming this, we can **immediately** deallocate and drop objects in
an unprotected scope. However, even in an unprotected scope, the `defer_free` and `defer_drop`
functions defers freeing and dropping the objects until the scope is over. Even worse, if a lot of
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be a good idea to simply remove defer_free and defer_drop from UnprotectedScope?

Why would someone want to call defer_free or defer_drop in an unprotected scope?


- Creating
the
[`Scope` trait](https://github.com/jeehoonkang/crossbeam-epoch/blob/unprotected/src/realm.rs#L15),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a split between EpochScope and UnprotectedScope, why not just put is_unprotected: bool inside Scope?

@ghost
Copy link

ghost commented Sep 4, 2017

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
Copy link
Contributor Author

jeehoonkang commented Sep 5, 2017

@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
Copy link

ghost commented Sep 5, 2017

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
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

1 participant