Out of band crate evaluation for 2017-07-06: threadpool

Crate evaluation for 2017-07-06: threadpool

For additional contribution opportunities, see the main libz blitz thread.

This post is a wiki. Feel free to edit it.

Links

Needs your help!

Anything that is not checked off still needs your help! There is no need to sign up or ask for permission - follow any of these links and leave your thoughts:

Guidelines checklist

Legend

  • [y] = guideline is adhered to, no work needed.
  • [n] = guideline may need work, see comments nearby
  • [/] = guideline not applicable to this crate

Checklist

Guidelines checklist

This document is collaboratively editable. Pick a few of the guidelines, compare the threadpool crate against them, and fill in the checklist with [y] if the crate conforms to the guideline, [n] if the crate does not conform, and [/] if the guideline does not apply to this crate. Each guideline is explained in $

  - [n] Crate name is a palindrome (C-PALINDROME)
   - error-chain backwards is niahc-rorre which is not the same as error-chain

Cookbook recipes

Cookbook example ideas

Come up with ideas for nice introductory examples of using threadpool, possibly in combination with other crates, that would be good to show in the Rust Cookbook. Please leave a comment in that issue with your ideas! You don't necessarily have to write the example code yourself but PRs are always welcome.

API guideline updates

What lessons can we learn from threadpool that will be broadly applicable to other crates? Please leave a comment in that issue with your ideas!

Discussion topics

Anything thatā€™s not a concrete crate issue yet. We want to eventually promote any topics here into actionable crate issues below.

Crate issues

How are we tracking?

Pre-review checklist

Do these before the libs team review.

  • Create evaluation thread based on this template
  • Work with author and issue tracker to identify known blockers
  • Compare crate to guidelines by filling in checklist
  • Record other questions and notes about crate
  • Draft several use case statements to serve as cookbook examples
  • Record recommendations for updated guidelines

Post-review checklist

Do these after the libs team review.

  • Publish libs team meeting video
  • Create new issues and tracking issue on crate's issue tracker
  • Solicit use cases for cookbook examples related to the crate
  • File issues to implement cookbook examples
  • File issues to update guidelines
  • Post all approachable issues to TWiR call for participation thread
  • Update links in coordination thread
6 Likes

Thanks @ericho. Iā€™m just here to bump this thread for now, but Iā€™ll be backā€¦

During reading of the documentation, I asked myself what the semantics of ThreadPool::clone() are. Does it spawns new threads or is it more like a reference to the existing ThreadPool?

When does it spawn its threads? On creation or on demand?

Iā€™ve completed the missing checkpoints in the etherpad. But there are three things that i donā€™t know where to put:

  • In contrast to crossbeam::scope, this crate requires the worker closure to be static. I think there should be an example on how to deal with the borrow checker error if the user tries to use some stack variable. -> Box

  • The Readme should explain the difference between the Similar libraries and itself, especially the scoping difference. It should also mention the pros of a non-scoped thread pool. (The word scoping is potentially unfamiliar to the user)

  • The usage section in the Readme does only show half the usage. Rename that section into Setup or give more details about usage in code?

3 Likes

Thanks @Phaiax for the checklist update :slight_smile:

I think the three items mentioned could be tracked with issues, the first will probably be into the cookbook issue and the other two in threadpool repo.

However weā€™ll need more discussion before go to the next steps.

The crate layout itself is a bit non-standard, where lib.rs lives in the root instead of under /src. Looks like it was changed in an appropriately named PR: Cleanup and other bikeshedding.

Is the idea that because this library is self-contained you can easily compile it sans-cargo like rustc --crate-type lib ./lib.rs? Thatā€™s also true of scoped-threadpool-rs, but not of crossbeam. None of the other libraries mentioned under Similar libraries have this constraint though. They can all be build with a simple rustc invocation. I still think itā€™s a nice feature though.

I think we should also compare this to futures_cpupool, which is often mentioned along with futures-rs and tokio. When should people use this and when should people use that?

1 Like

I added the lib.rs at the root level since weā€™re only working with a single file, the src/ directory felt a little overkill, but Iā€™m happy to move it back to src/lib.rs for the sake of consistency across Rust crates.

Do you find it confusing enough that we should get rid of the Clone implementation? I've never personally used clone on a ThreadPool, and am not entirely certain what the use-case is. We could also just add documentation to the trait implementation.

Tangential though, there's an open PR for adding more trait implementations that could use some feedback:

I agree there should be some distinction or description about the threadpool in the ā€˜threadpoolā€™ crate and scoped threadpools in the ā€˜similar librariesā€™ section of the readme. Thereā€™s a pretty good description of what a scoped thread is in the crossbeam crate.

For what itā€™s worth, Iā€™m not opposed to extending the ā€˜threadpoolā€™ crate to include a scoped threadpool in addition to the existing one. How do people feel about that? The crate could be an all-in-one sort of solution

I think a scoped threadpool wouldnā€™t go astray, if thatā€™s additional complexity and scope (get it?) youā€™re willing to accept.

Some other random thoughts:

  • Having a quick poke around crates.io, threadpool looks like the most depended on thread pool library.
  • I think the big difference between threadpool and futures-cpupool is the use of futures for driving units of work. Futures introduces more complexity, but if youā€™re already in the context of a future then it makes sense to use them.

Implementing the open features is fun. I will open another PR and implement Clone by hand. That way I can add documentation to it.

EDIT: https://github.com/frewsxcv/rust-threadpool/pull/79

2 Likes

Iā€™m wondering if the suggestion of the scoped threadpool is out of the purpose of this evaluation. I mean, the purpose of this effort is to get the crate in a better shape, so iā€™m not sure if a new feature fits here unless is a blocking issue.

I've used clone a bit on the futures-cpupool so it can be shared by multiple things that want to offload work. I think it's a reasonable thing to do in the way the proposed implementation does it. The docs for futures-cpupool have this to say:

Currently CpuPool implements Clone which just clones a new reference to the underlying thread pool.

You're right it could well be out of scope, but I think it's a good opportunity to explore the possibility. It could be added in the future as a non-breaking change so I don't think we need to block on it.

I think with the current behavior of clone we get a reasonable interface for sharing an entrypoint to the pool over many threads. While developping the join feature I noticed that moving the Sender (see docs) from the job-queue between too many threads will cause a panic at some point because it is not Sync. So using an Arc to move the pool around will lead to problems.

I will revisite the implementation of the bookkeeping with the input I received from other people.

Probably less related: We managed to finish post-processing my talk about the threadpool. You can watch it here:

From the etherpad:

Since it [ThreadPool] is Clone, it could implement Eq. But I can not figure out a use case. * use case: image you have many pools and you pass them around. At some point you would like to know if two entrypoints work on the same pool.

Hmm, this seems a bit strange to me. Even if you had the knowledge that 2 ThreadPools pointed to the same pool I'm not sure what you could really do with that information.

At least it seems very niche, and something you could do by wrapping a ThreadPool in a newtype along with an identifier if you really need it.

Ah nevermind, I see ThreadPool is actually already Eq.

Just opened a PR that adds a Builder with the intention of removing the custom constructors (like ThreadPool::with_name) in the future. https://github.com/rust-threadpool/rust-threadpool/pull/83

I think the PartialEq, Eq, and Clone implementations on the ThreadPool are a bit strange and very niche. Iā€™m thinking for a 2.0.0 release, Iā€™ll remove them and if the user needs that functionality, all they need to do is wrap the ThreadPool in an Arc.

Iā€™m not sure Clone should be replaced with Arc. I donā€™t think thereā€™s anything niche about sending handles to a threadpool to threads. If you put in Arc, then any &mut self methods are uncallable.

PartialEq and Eq, however, indeed seem odd.

1 Like

In my opinion Clone and PartialEq go hand in hand. Once you can have more than one entrypoint to a pool you will most likely want to compare it with other entrypoints to be able to tell them apart.

An Arc is not an option here, because we use a channel internally, see example