The FILE thread is dead, long live the blocking pool
Brett Wilson
operations that backup on this thread has become a more significant
problem. I recently added the base::SequencedWorkerPool which is
available via BrowserThread::GetBlockingPool() to be a replacement for
random blocking I/O that you don't want to run on the UI/IO threads.
For common cases and for ease-of-conversion of existing code, there
are two helper methods:
- BrowserThread::PostBlockingPoolTask for posting tasks to the pool
with no ordering (tasks will overlap) and the default shutdown
semantics (will block shutdown).
- BrowserThread::PostBlockingPoolSequencedTask for posting tasks with
ordering enforced by the given string identified.
- Use the underlying API (via BrowserThread::GetBlockingPool) to do
sequencing without having to have unique strings, or to control
shutdown semantics.
New code should use these helper methods or the raw worker pool
instead, and we should try to convert existing code for better
latency. The header file base/threading/sequenced_worker_pool.h should
be well documented. Some highlights:
* Normally you post tasks and they're executed potentially in parallel
on a number of threads.
* If your component needs sequencing between tasks, you get a sequence
token or a unique string. All tasks posted with the same sequence
token/string will execute in sequence.
* If you're converting a larger component like the download manager
where there may be implicit ordering assumptions between many files,
use a string shared by all those files to sequence those tasks.
* Unlike normal threads, it supports different behavior on shutdown.
The default behavior is to run your task and block shutdown. If your
task isn't critical, you can specify that it can be discarded if it
hasn't started by the time we start shutdown. If you have written your
code to be OK if the browser shuts down in the middle of its
execution, you can also specify that we ship joining with the thread
if your task is running (this is useful for simple but potentially
very slow things like DNS queries).
Brett
Evan Martin
> The FILE thread's uses have expanded to cover so many random blocking
> operations that backup on this thread has become a more significant
> problem. I recently added the base::SequencedWorkerPool which is
> available via BrowserThread::GetBlockingPool() to be a replacement for
> random blocking I/O that you don't want to run on the UI/IO threads.
Do you have any comments on how to prevent a storm of disk operations
on startup?
I know that sequencing all disk operations on a single thread isn't
necessarily the best, but from your high-level description I'd worry
that 18 different subsystems are now slamming the disk in parallel
during startup.
Brett Wilson
>
> A twinge of fear arises in me upon reading this, as it ratchets up many
> notches the complexity and requisite skill for writing correct code.
>
> Asserting one is on the file thread is a pretty simply way to dodge
> concurrency related errors. In converting such code, is there a way to
> assert that one is running under a particular "ordering string" to cover the
> simple cases?
>
> Of course, code expected to be called under multiple ordering strings will
> need to now do its own locking -- with all that that entails. Are our
> locking facilities sophisticated enough to handle all this (e.g. enforcing a
> lock acquisition order to avoid deadlocks, diagnosing hung threads, and the
> like)?
You should not write such code. Instead, use the same sequence token
for all the requests in your component.
Brett
Brett Wilson
> don't both call into some shared facility under different tokens?
Can you give an example of some code that would have this problem?
Generally things that post to the file thread execute a simple closure
to do some I/O. Otherwise, either components are threadsafe or not
threadsafe. There are few if any existing non-threadsafe components
that live on the file thread today. If any such things exist, they
will obviously need to be redesigned if code in the pool wants to use
it.
Brett
Peter Kasting
New code should use these helper methods or the raw worker pool
instead, and we should try to convert existing code for better
latency.
Michael Nordman
Brett
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
Brett Wilson
> I can think of a few things to work out as this get adopted...
>
> 1) BrowserMessageFilter::OverrideThreadForMessage could use a way to
> indicate a 'SequenceToken' (or any worker pool thread) in place of one of
> the well-known thread IDs.
>
> 2) Maybe some kind of 'MockSequencedWorkerPoll' capability that schedules
> tasks on the current thread for use in unittests. So the code being tested
> can post tasks and then the test harness can call loop.RunAllPending()... no
> extra threads or thread sychronization for testing required.
Although not exactly the same, you can call
BrowserThread::GetBlockingPool()->FlushForTest() which will block
until all posted work is done.
Brett
William Chan (陈智昌)
On Friday, January 20, 2012, Michael Nordman <michaeln@google.com> wrote:
> I can think of a few things to work out as this get adopted...
> 1) BrowserMessageFilter::OverrideThreadForMessage could use a way to indicate a 'SequenceToken' (or any worker pool thread) in place of one of the well-known thread IDs.
> 2) Maybe some kind of 'MockSequencedWorkerPoll' capability that schedules tasks on the current thread for use in unittests. So the code being tested can post tasks and then the test harness can call loop.RunAllPending()... no extra threads or thread sychronization for testing required.
This is one reason I favor using a TaskRunner interface, which would allow for easy dependency injection. As I discussed previously with Brett, we could pass around TaskRunner pointers instead of concrete tokens. You can imagine a TaskRunner implementation that contains the sequence token and passes through to the worker pool, so all operations that should share the same sequence token would share the same TaskRunner instead, and then you get dependency injection capability too. And since TaskRunner would layer on top of MessageLoopProxy and WorkerPool too, you can easily swap the desired task execution behavior. Of course, this requires a bit more work. I meant to get around to it before my leave, but maybe I'll do it next month when I'm back.
> 3) Maybe plug an instance into test_shell/ DRT somewhere.
>
>
Mattias Nissler
On Friday, January 20, 2012, Michael Nordman <michaeln@google.com> wrote:
> I can think of a few things to work out as this get adopted...
> 1) BrowserMessageFilter::OverrideThreadForMessage could use a way to indicate a 'SequenceToken' (or any worker pool thread) in place of one of the well-known thread IDs.
> 2) Maybe some kind of 'MockSequencedWorkerPoll' capability that schedules tasks on the current thread for use in unittests. So the code being tested can post tasks and then the test harness can call loop.RunAllPending()... no extra threads or thread sychronization for testing required.This is one reason I favor using a TaskRunner interface, which would allow for easy dependency injection. As I discussed previously with Brett, we could pass around TaskRunner pointers instead of concrete tokens. You can imagine a TaskRunner implementation that contains the sequence token and passes through to the worker pool, so all operations that should share the same sequence token would share the same TaskRunner instead, and then you get dependency injection capability too. And since TaskRunner would layer on top of MessageLoopProxy and WorkerPool too, you can easily swap the desired task execution behavior. Of course, this requires a bit more work. I meant to get around to it before my leave, but maybe I'll do it next month when I'm back.
Darin Fisher
On Fri, Jan 20, 2012 at 7:05 AM, William Chan (陈智昌) <will...@chromium.org> wrote:
On Friday, January 20, 2012, Michael Nordman <michaeln@google.com> wrote:
> I can think of a few things to work out as this get adopted...
> 1) BrowserMessageFilter::OverrideThreadForMessage could use a way to indicate a 'SequenceToken' (or any worker pool thread) in place of one of the well-known thread IDs.
> 2) Maybe some kind of 'MockSequencedWorkerPoll' capability that schedules tasks on the current thread for use in unittests. So the code being tested can post tasks and then the test harness can call loop.RunAllPending()... no extra threads or thread sychronization for testing required.This is one reason I favor using a TaskRunner interface, which would allow for easy dependency injection. As I discussed previously with Brett, we could pass around TaskRunner pointers instead of concrete tokens. You can imagine a TaskRunner implementation that contains the sequence token and passes through to the worker pool, so all operations that should share the same sequence token would share the same TaskRunner instead, and then you get dependency injection capability too. And since TaskRunner would layer on top of MessageLoopProxy and WorkerPool too, you can easily swap the desired task execution behavior. Of course, this requires a bit more work. I meant to get around to it before my leave, but maybe I'll do it next month when I'm back.
+1 to this idea. People around me have repeatedly run into the need of mocking out (delayed) task posting in tests, and the TaskRunner interface you describe would support that use case perfectly as far as I understand.
Achuith Bhandarkar
Brett Wilson
<ach...@chromium.org> wrote:
> Hello,
>
> Should I be asking people to use blocking pool instead of the FILE thread in
> code reviews?
Ideally, yes in simple cases. I don't think it needs to be a
requirement that you convert a large component over when just doing a
small change, though.
> I find DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE) very useful
> for catching threading errors. Is there something similar for the blocking
> pool?
No, it would like to add something like that, but it's a little more
involved than the current code. I think it would mean somehow marking
all of the threads and have a special CurrentlyOnBlockingPool check,
or something.
> I think Peter's question went unanswered - Is there a plan to migrate our
> existing FILE thread calls to the blocking Pool similar to the effort for
> base::Bind?
Volunteers welcome!
Brett
John Abd-El-Malek
William Chan (陈智昌)
Sent from my iNexus
On Feb 13, 2012 1:08 PM, "John Abd-El-Malek" <j...@chromium.org> wrote:
>
>
>
> On Mon, Feb 13, 2012 at 12:11 PM, Brett Wilson <bre...@chromium.org> wrote:
>>
>> On Mon, Feb 13, 2012 at 11:48 AM, Achuith Bhandarkar
>> <ach...@chromium.org> wrote:
>> > Hello,
>> >
>> > Should I be asking people to use blocking pool instead of the FILE thread in
>> > code reviews?
>>
>> Ideally, yes in simple cases. I don't think it needs to be a
>> requirement that you convert a large component over when just doing a
>> small change, though.
>>
>> > I find DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE) very useful
>> > for catching threading errors. Is there something similar for the blocking
>> > pool?
>>
>> No, it would like to add something like that, but it's a little more
>> involved than the current code. I think it would mean somehow marking
>> all of the threads and have a special CurrentlyOnBlockingPool check,
>> or something.
>>
>> > I think Peter's question went unanswered - Is there a plan to migrate our
>> > existing FILE thread calls to the blocking Pool similar to the effort for
>> > base::Bind?
>>
>> Volunteers welcome!
>
>
> I think it would be much better to wait to convert code until we have a TaskRunner/Executer interface (google3 has this abstraction and it's very useful there).
Sorry, akalin has done it already, I will finish the review when I get back from whistler tonight.
