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

Unify Guard and PoolGuard types #30

Open
bIgBV opened this issue Mar 31, 2020 · 5 comments
Open

Unify Guard and PoolGuard types #30

bIgBV opened this issue Mar 31, 2020 · 5 comments
Labels
enhancement New feature or request

Comments

@bIgBV
Copy link
Collaborator

bIgBV commented Mar 31, 2020

Currently Slab::get returns the Guard type while Pool::get returns a PoolGuard. But looking at the contents of these two types, we can see that they carry the same information. It would be a great win for users if we are able to unify these types.

The reason for the difference for the two types comes to their Drop implementations:

  • Here is the Drop impl for Guard

sharded-slab/src/lib.rs

Lines 499 to 511 in ffa5c7a

impl<'a, T, C: cfg::Config> Drop for Guard<'a, T, C> {
fn drop(&mut self) {
use crate::sync::atomic;
if self.inner.release() {
atomic::fence(atomic::Ordering::Acquire);
if Tid::<C>::current().as_usize() == self.shard.tid {
self.shard.remove_local(self.key);
} else {
self.shard.remove_remote(self.key);
}
}
}
}

  • And for PoolGuard

impl<'a, T, C> Drop for PoolGuard<'a, T, C>
where
T: Clear + Default,
C: cfg::Config,
{
fn drop(&mut self) {
use crate::sync::atomic;
test_println!(" -> drop PoolGuard: clearing data");
if self.inner.release() {
atomic::fence(atomic::Ordering::Acquire);
if Tid::<C>::current().as_usize() == self.shard.tid {
self.shard.mark_clear_local(self.key);
} else {
self.shard.mark_clear_remote(self.key);
}
}
}
}

Both these impls, while calling different methods, are calling the "mark for release" chain. The final guard being dropped is responsible for actually clearing the storage. The calls eventually lead to Slot::try_remove_value for Guards drop impl and Slot::try_clear_storage for PoolGuards drop impl. The sole difference comes down to the mutator parameter being passed to the release_with method for the Slab and Pool impls.

The mutator closure in Pools case calls the Clear::clear on T, therefore we need the T: Clear bound to be true. In the Slabs case we need the type to be an Option<T> in order to call Option::take.

If we can find a way to scoping those two pieces of functionality to impl block bounded by the specific generic types we will be able to expose a unified guard type. Not only that but we will be able to share a lot more code throughout the crate as a lot of the functions in this particular code path are repetitive.

@bIgBV bIgBV added the enhancement New feature or request label Mar 31, 2020
@hawkw
Copy link
Owner

hawkw commented Mar 31, 2020

In the Slabs case we need the type to be an Option<T> in order to call Option::take.

Since we're not actually returning the taken item in this code path, can't we just use the Clear impl for Option?

impl<T> Clear for Option<T> {
fn clear(&mut self) {
let _ = self.take();
}
}

This should have equivalent behavior to the current implementation. I think it should be fine to have one code path that's used by both guards. It's only Slab::take that requires special-casing.

@hawkw hawkw added this to the Release-Ready Object Pool milestone Mar 31, 2020
@hawkw
Copy link
Owner

hawkw commented Mar 31, 2020

@bIgBV Are you interested in working on this? I'd like this to be addressed before we publish a release with the object pool API. If you're busy, I'm happy to take care of it so we can move closer to a release.

@bIgBV
Copy link
Collaborator Author

bIgBV commented Mar 31, 2020

@hawkw yeah I'd like to work on this. And the Clear impl for Option looks like a great way to move forward with this.

@hawkw
Copy link
Owner

hawkw commented Mar 31, 2020

Okay, great. I think this is the last release blocker for the pool.

@hawkw
Copy link
Owner

hawkw commented Apr 2, 2020

Looks like this probably isn't going to be possible with the current state of things (see #33 (comment) and #33 (comment)) so I'm not going to block the release on it.

@hawkw hawkw removed this from the Release-Ready Object Pool milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants