Initial implementation of threadpool::Builder.

[frewsxcv]

Related to #77.

[frewsxcv]

todo: make this work

[dns2utf8]

I would call it PoolBuilder or even ThreadPoolBuilder to avoid conflicts.

[frewsxcv]

I chose Builder to match the one in the standard library https://doc.rust-lang.org/std/thread/struct.Builder.html
The user can always rename the import if it conflicts. I don't feel strongly though and am open to renaming

[dns2utf8]

Good point. I was thinking about the thread_name. What do you think about worker_name or something like that?
My current reasoning is

• Currently the workers get just the name from the pool. They could be enumarated like {poolname}-{worker_thread_number}
• Pushing it one step further the workers could increment their count with every job.
• Finally we could extend the naming of the workers with a RenamePool message. So renaming the pool would happen like that:

let pool = Builder. ... set_pool_name("first name");
pool.execute( ... ); // Worker with the first name
pool.rename("some new name");
pool.execute( ... ); // once this job is scheduled the worker will rename itself

I think, renaming a thread currently requires to spawn a new thread with the stable branch.

[mjkillough]

It would be nice if this and other references to ThreadPool were made into links to struct.ThreadPool.html, so that the C-LINK guideline was met. The existing documentation is updated in #85.

[frewsxcv]

Good point. I was thinking about the thread_name. What do you think about worker_name or something like that?
My current reasoning is

• Currently the workers get just the name from the pool. They could be enumarated like {poolname}-{worker_thread_number}
• Pushing it one step further the workers could increment their count with every job.
• Finally we could extend the naming of the workers with a RenamePool message. So renaming the pool would happen like that:

let pool = Builder. ... set_pool_name("first name");
pool.execute( ... ); // Worker with the first name
pool.rename("some new name");
pool.execute( ... ); // once this job is scheduled the worker will rename itself

I thinkg, renaming a thread currently requires to spawn a new thread with the stable branch.

Interesting ideas! I'm not sure what distinction you're making between the 'worker' and 'thread' names here. Are these ideas for this PR or could they be done in a follow-up PR?

[dns2utf8]

The renaming can be implemented later. I think it would only require the channel to transmit an enum with the commands and the closures.
On a related note, I opened a PR on rustc to rename threads so a rename would be more efficient than spawning new workers after every rename. Unfortunately it will take at least until rust 1.22.0 until it lands in stable.

If I recall correctly I made the distinction to thread_name because to me it felt like controlling the actual or main thread. worker_name felt more like telling the pool to make sure all the worker threads will have the specified name.

[dns2utf8]

I would prefer the constructing function to be reusable/non-consuming. In case somebody would like to create multiple pools with a similar configuration.

[dns2utf8]

Did you consider calling it build or create?

[frewsxcv]

I like build. I'll change it to that

[frewsxcv]

I would prefer the constructing function to be reusable/non-consuming. In case somebody would like to create multiple pools with a similar configuration.

fair enough. added Clone to Builder.

see anything else?

[dns2utf8]

pub fn build(&self) -> ThreadPool

[frewsxcv]

that would cause unnecessary allocations. if the user needs to build another threadpool Builder (most do not), they can just call clone on the Builder.

[dns2utf8]

After thinking about it a bit more, leaving the Builder to the caller after build with &self gives a similar result without allocations of more memory.

I don't have a strong opinion on Clone.

Apart from that: 👍 looks very good to me. Thank you!

[frewsxcv]

this is what the code looks like if we changed build to &self:

diff --git a/src/lib.rs b/src/lib.rs
index 6a78abf..a889008 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -278,13 +278,13 @@ impl Builder {
     ///     .thread_stack_size(4_000_000)
     ///     .build();
     /// ```
-    pub fn build(self) -> ThreadPool {
+    pub fn build(&self) -> ThreadPool {
         let (tx, rx) = channel::<Thunk<'static>>();
 
         let num_threads = self.num_threads.unwrap_or_else(num_cpus::get);
 
         let shared_data = Arc::new(ThreadPoolSharedData {
-            name: self.thread_name,
+            name: self.thread_name.clone(),
             job_receiver: Mutex::new(rx),
             empty_condvar: Condvar::new(),
             empty_trigger: Mutex::new(()),

this means that if someone did Builder::new().thread_name("foo".into()).build(), the string would be allocated twice instead of once.

[dns2utf8]

Ah, good point. I did not think of that. Merge?

[frewsxcv]

bors r+

[frewsxcv]

bors r+

[frewsxcv]

bors r+

[notriddle]

bors ping

[dns2utf8]

bors r+

Can I talk to bors too?

[notriddle]

You should be able to, but it's not working. Probably related to moving this repo from @frewsxcv's account to the organization.

[frewsxcv]

bors r+

[frewsxcv]

bors r+

[bors]

Timed out

[dns2utf8]

bors retry

[notriddle]

Has AppVeyor been turned on yet?

[frewsxcv]

bors r+

[frewsxcv]

@notriddle That was it. Appveyor didn't track the rename so it was not longer enabled. Wonder if there's a better way to handle this, maybe bors should add a comment

[notriddle]

I know what bors needs to do to handle the rename, roughly speaking. And I can't fix AppVeyor.

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push