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

[Phaiax]

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?

[Phaiax]

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?

[ericho]

Thanks @Phaiax for the checklist update

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.

[KodrAus]

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.

[crlf0710]

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?

[frewsxcv]

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.

[frewsxcv]

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:

Common traits PartialEq, Eq and Hash

[frewsxcv]

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

[KodrAus]

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.

[dns2utf8]

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

[ericho]

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.

[KodrAus]

I’ve never personally used clone on a ThreadPool, and am not entirely certain what the use-case is.

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.

I’m wondering if the suggestion of the scoped threadpool is out of the purpose of this evaluation.

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.

[dns2utf8]

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:

• Direct link
• Youtube
• Mozilla Air

[stjepang]

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.

Somewhat related to the topic, but I’d like to chime in: I’m working on a new channel that allows sharing both Senders and Receivers. It is still a work-in-progress, but will be finished soon and moved into Crossbeam.

To have a taste of its features, take a look at this example, where I converted a program in Go written in CSP-style into Rust.

Here’s what else it can do.

[KodrAus]

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.

[KodrAus]

Ah nevermind, I see ThreadPool is actually already Eq.

[frewsxcv]

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.

[vitalyd]

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.

[dns2utf8]

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

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.