search

hawkw / sharded-slab

a lock-free concurrent slab (experimental)
MIT License
270 stars 19 forks source link

Unify `Guard` and `PoolGuard` types #30

Open bIgBV opened 4 years ago

bIgBV commented 4 years ago

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:

https://github.com/hawkw/sharded-slab/blob/ffa5c7a0eeb576c159b86a700c00bb1adf2a812c/src/lib.rs#L499-L511

https://github.com/hawkw/sharded-slab/blob/ffa5c7a0eeb576c159b86a700c00bb1adf2a812c/src/pool/mod.rs#L232-L249

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.

hawkw commented 4 years ago

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? https://github.com/hawkw/sharded-slab/blob/ffa5c7a0eeb576c159b86a700c00bb1adf2a812c/src/clear.rs#L15-L19 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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

hawkw commented 4 years ago

Looks like this probably isn't going to be possible with the current state of things (see https://github.com/hawkw/sharded-slab/pull/33#issuecomment-607039927 and https://github.com/hawkw/sharded-slab/pull/33#issuecomment-607352267) so I'm not going to block the release on it.