Page MenuHomePhabricator

[Support] ThreadPool: Don't spawn any threads when ThreadCount = 1
Needs ReviewPublic

Authored by vsk on Oct 19 2016, 11:55 AM.

Details

Summary

This should save clients the time/memory overhead of spawning a thread when ThreadCount/std::thread::hardware_concurrency() = 1. IMO this is also a de facto simplification of the ThreadPool API, because clients no longer have to maintain slightly separate code paths for the ThreadCount = 1 / ThreadCount > 1 cases.

Tested by running unittests/Support/SupportTests configured with -DLLVM_ENABLE_THREADS={On,Off}.

Diff Detail

Event Timeline

vsk updated this revision to Diff 75185.Oct 19 2016, 11:55 AM
vsk retitled this revision from to [Support] ThreadPool: Don't spawn any threads when ThreadCount = 1.
vsk updated this object.
vsk added a subscriber: llvm-commits.

Thinking a bit more about it, I'm not sure it is intuitive: the client could do something else after issuing a task to the pool. After this patch this computation would no longer occur in parallel.

mehdi_amini removed a subscriber: mehdi_amini.
vsk added a comment.Oct 19 2016, 12:45 PM

Thinking a bit more about it, I'm not sure it is intuitive: the client could do something else after issuing a task to the pool. After this patch this computation would no longer occur in parallel.

Fair enough. Do you think the sequential behavior makes sense when ThreadCount = 0? I.e, should we make ThreadPool(0) behave the same way regardless of whether LLVM_ENABLE_THREADS=On|Off? Currently, the sequential behavior does not seem possible when LLVM_ENABLE_THREADS=On.

mehdi_amini edited edge metadata.Oct 19 2016, 12:50 PM

That seems fine and makes sense to me from an API point of view.
I'm not sure how to translate this to the user though: I don't expect someone to run -threads=0. And we're back to special casing in the client: ThreadPool Pool(ThreadCount == 1 ? 0 : ThreadCount); which isn't great either.

vsk added a comment.Oct 19 2016, 12:58 PM

That seems fine and makes sense to me from an API point of view.
I'm not sure how to translate this to the user though: I don't expect someone to run -threads=0. And we're back to special casing in the client: ThreadPool Pool(ThreadCount == 1 ? 0 : ThreadCount); which isn't great either.

Good point. Wdyt of adding 'bool PreferSequential = false' to the constructors? The meaning of 'PreferSequential' would be ThreadCount = (PreferSequential && MaxThreadCount == 1) ? 0 : MaxThreadCount. That way, users can opt-in to executing all the tasks in wait().

In D25784#574651, @vsk wrote:

That seems fine and makes sense to me from an API point of view.
I'm not sure how to translate this to the user though: I don't expect someone to run -threads=0. And we're back to special casing in the client: ThreadPool Pool(ThreadCount == 1 ? 0 : ThreadCount); which isn't great either.

Good point. Wdyt of adding 'bool PreferSequential = false' to the constructors? The meaning of 'PreferSequential' would be ThreadCount = (PreferSequential && MaxThreadCount == 1) ? 0 : MaxThreadCount. That way, users can opt-in to executing all the tasks in wait().

That looks weird to create a threads pool and saying at the same time that I "prefer sequential": ThreadPool Pool(MaxThreadCount, /* PreferSequential */ true); ; it's almost contradictory (though I see how it could simplify the client code somehow).

This "optimization" seems like a difficult thing to fit in the API. I'm not very inspired right now, so I haven't any great suggestion.

By the way, did you have a particular client in mind? The pool isn't really designed for very lightweight task but rather "heavier" one, where the cost of spawning a thread shouldn't be significant?

vsk added a comment.Oct 19 2016, 2:40 PM

Good point. Wdyt of adding 'bool PreferSequential = false' to the constructors? The meaning of 'PreferSequential' would be ThreadCount = (PreferSequential && MaxThreadCount == 1) ? 0 : MaxThreadCount. That way, users can opt-in to executing all the tasks in wait().

That looks weird to create a threads pool and saying at the same time that I "prefer sequential": ThreadPool Pool(MaxThreadCount, /* PreferSequential */ true); ; it's almost contradictory (though I see how it could simplify the client code somehow).

Hm, yeah.

This "optimization" seems like a difficult thing to fit in the API. I'm not very inspired right now, so I haven't any great suggestion.

By the way, did you have a particular client in mind? The pool isn't really designed for very lightweight task but rather "heavier" one, where the cost of spawning a thread shouldn't be significant?

I'll try and think of a more natural way to do this. Maybe adding support for the ThreadCount = 0 case is the simplest option.

The client I had in mind is llvm-cov, where each thread writes out a coverage report to disk. It is not relatively expensive to spawn a thread in this context. But it would be nice to just have one code path for the threaded+non-threaded cases instead of two.

Yeah, right now supporting ThreadCount == 0 seems like the most straightforward and less intrusive way to get it in the API.

The client I had in mind is llvm-cov, where each thread writes out a coverage report to disk. It is not relatively expensive to spawn a thread in this context. But it would be nice to just have one code path for the threaded+non-threaded cases instead of two.

If spawning a thread is not relatively expensive, why would you bother having a separate code path?

vsk added a comment.Oct 19 2016, 2:53 PM

If spawning a thread is not relatively expensive, why would you bother having a separate code path?

I got a report about thread-related flakiness in llvm-cov (PR30735), and thought that avoiding thread creation could mitigate it.

I may miss something, but I'm seeing this "hiding a latent bug", and as such is not a great motivation (it means that there might be a latent bug with the threading infrastructure and/or llvm-cov, possibly on a particular platform, but you're reducing the coverage on this aspect).

vsk added a comment.Oct 19 2016, 5:03 PM

I may miss something, but I'm seeing this "hiding a latent bug", and as such is not a great motivation (it means that there might be a latent bug with the threading infrastructure and/or llvm-cov, possibly on a particular platform, but you're reducing the coverage on this aspect).

In this particular case (PR30735), we still have tests that stress the multi-threaded path on the affected platform, so the coverage hasn't been removed altogether. I don't expect this kind of change to eradicate the bug, and am still searching for the root cause.

Separately, I still think it should be possible to avoid paying the thread creation overhead when using ThreadPool with LLVM_ENABLE_THREADS=On.

Maybe the ThreadPool Pool(ThreadCount == 1 ? 0 : ThreadCount); is the best we can do for now?