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 Recycler to compliment Supplier #21

Open
vitiral opened this issue Nov 14, 2018 · 4 comments
Open

add Recycler to compliment Supplier #21

vitiral opened this issue Nov 14, 2018 · 4 comments

Comments

@vitiral
Copy link

vitiral commented Nov 14, 2018

Supplier is used to construct new instances, however the user may also want to control the maximum size allowed when items go back into the pool to avoid OOM.

I recommend the name Recycler, although you can use whatever name you think is fitting.

use case:

extern crate lifeguard;
use lifeguard::*;

fn main() {
 let pool : Pool<String> = pool()
   // The pool will allocate 128 values for immediate use. More will be allocated on demand.
   .with(StartingSize(128))
   // The pool will only grow up to 4096 values. Further values will be dropped.
   .with(MaxSize(4096))
   // The pool will use this closure (or other object implementing Supply<T>) to allocate
   .with(Supplier(|| String::with_capacity(1024)))
   // The pool will use this closure when adding recycled items back to the pool
   .with(Recycler(|s| {
        s.clear()
        // nightly only
        if s.capacity() > 4096 {
            s.shrink_to(4096); 
        }
    })
   .build();
  // ...
}
@vitiral
Copy link
Author

vitiral commented Nov 14, 2018

this brings up another good point, which is that if you don't have a Recycler to properly clean the classes, couldn't you receive a String that is full of data?

@zslayton
Copy link
Owner

Thanks for opening an issue. I love the idea of having a Recycler type! It would address two problems that the library currently has:

  1. Because users have to implement Recycleable for T, they can't use this library with types that they didn't define without opening a pull request to either the type's library or to lifeguard. This is an obvious stumbling block.
  2. Implementing Recycleable means that you can only recycle each type in one way. I can easily imagine situations where you'd tune the recycling process to fit a use case as you've illustrated above.

However, I have one concern that will take some work to address. The Supplier specifies an implementation of the Supply trait and is stored in the Pool as a Box<Supply> trait object. This means that each time we use it, virtual dispatch is required. This isn't that big a penalty because it only happens when the Pool is empty and another value is requested, which should be somewhat infrequent over the life of the Pool. With a Recycler, however, we'd be using virtual dispatch every time a value from dropped and needed to return to the pool, which is a very frequent occurrence. We'd need to do some benchmarking to see how much of a problem this would really be -- I might be worrying about nothing.

This makes me wonder whether I should replace the Recycleable trait with a Recycler trait. Pool<T> would become Pool<R> where R: Recycler<T>. Users could define custom behavior for any/all types and the library could avoid virtual dispatch altogether.

Today's

let pool: Pool<String> = pool()
    .with(Supplier(|| String::new()))
    .with(StartingSize(16))
    .with(MaxSize(128))
    .build();

would become

let pool: Pool<StringRecycler> = pool(my_string_recycler)
    .with(StartingSize(16))
    .with(MaxSize(128))
    .build();

lifeguard could provide common implementations of Recyclers out of the box, including Recyclers for Vec and String that would shrink unused capacity.

Whaddya think?

@zslayton
Copy link
Owner

this brings up another good point, which is that if you don't have a Recycler to properly clean the classes, couldn't you receive a String that is full of data?

This is currently handled by the Recycleable trait, which requires that implementors define how a value should be reset().

@vitiral
Copy link
Author

vitiral commented Nov 14, 2018

I don't fully follow (and don't have time to dig in, sorry!). Also thanks, I missed the Recyclable trait.

Your solution sounds good to me. If I follow, you could insert arbitrary types if you had a constructor that took the recycling function -- you wouldn't even need to implement the trait yourself!

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

No branches or pull requests

2 participants