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 a PlannerBuilder #104

Closed
wants to merge 3 commits into from
Closed

Add a PlannerBuilder #104

wants to merge 3 commits into from

Conversation

torkleyy
Copy link
Member

@torkleyy torkleyy commented Mar 4, 2017

This allows sharing the thread pool, which is pretty important for highly parallel applications in order to allow a good performance.

Sorry I've done so bad in my previous PR, I hope this one is better.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I wonder why you want to have a builder at all? There is only 2 ways to create a planner: with given thread pool or without. So it sounds like Planner could just have 2 different constructors, let's say new and with_thread_pool. Did you consider this?

src/planner.rs Outdated
/// threads, a new thread pool with the number of virtual
/// cpus will be created.
pub struct PlannerBuilder {
world: Option<World>,
Copy link
Member

Choose a reason for hiding this comment

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

what is the point of having an Option here in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I thought about using with_thread_pool, but I thought it's not descriptive enough because you still have another parameter. Yes, you are right I could have asked for world already in the builder constructor, but I think it is nicer to use. Of course it's not compile-time enforced then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus, now the pool size defaults to the number of cpus, which should be pretty common. I do have another solution in mind, though:

enum PoolOptions {
    Pool(Arc<ThreadPool>),
    NumThreads(usize),
    NumCpus,
}

impl Planner {
    pub fn from_options(options: PoolOptions) -> Self {
        
    }
}

@kvark
Copy link
Member

kvark commented Mar 5, 2017

How about we have just new going the NumCpu path and from_pool accepting a thread pool? that would be simple enough and covers all use-cases.

@torkleyy
Copy link
Member Author

torkleyy commented Mar 6, 2017

I found a non-breaking way, using a trait PoolSize.

@kvark
Copy link
Member

kvark commented Mar 6, 2017

@torkleyy I don't think it's worth it. Let's not have a fancy API, let's have a simple one, with new() and from_pool(...) constructors.

@torkleyy
Copy link
Member Author

torkleyy commented Mar 6, 2017

So you mean I should just take a plain usize?

@kvark
Copy link
Member

kvark commented Mar 6, 2017

No, I mean that we don't even need a constructor that takes usize. A most common case would be to use the number cpus, and that's what I propose for the new() constructor to do. A less common case would be to receive the Arc<ThreadPool>, which covers a case where you want a specific number of threads.

@nchashch
Copy link
Member

nchashch commented Mar 6, 2017

Maybe add a Planner::new, taking a usize and Planner::new_num_cpus. There are use cases where you don't need an Arc<ThreadPool> and you need to set the number of threads (for example to benchmark performance with and without multithreading).

And from the user's perspective it isn't obvious that the way you get a Planner with a set number of threads is call Planner::from_pool.

@torkleyy
Copy link
Member Author

torkleyy commented Mar 6, 2017

Closing because title does not match the content anymore. #106 replaces this PR

@torkleyy torkleyy closed this Mar 6, 2017
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

Successfully merging this pull request may close these issues.

3 participants