Hook up the SequencedWorkerPool to the browser thread.

Hook up the SequencedWorkerPool to the browser thread.

This does some refactoring of the static data in the browser thread so we only have one global object instead of a bunch fo separate arrays.

It also hooks up the visited link master's I/O to use this new system as a proof of concept.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116816

[jam, 2012-01-03 19:48:17]

http://codereview.chromium.org/9065009/diff/10/content/public/browser/browser...
File content/public/browser/browser_thread.h (right):

http://codereview.chromium.org/9065009/diff/10/content/public/browser/browser...
content/public/browser/browser_thread.h:164: static base::SequencedWorkerPool*
GetBlockingPool();
does this getter really belong on BrowserThread? it seems unrelated to the other
threads, since those use a very different model (i.e. BT::PostTask(UI, ...)) vs
this

[brettw, 2012-01-03 20:09:23]

Do you have an idea about where this would go better? On the g_browser_process?

[jam, 2012-01-03 21:01:09]

On 2012/01/03 20:09:23, brettw wrote:
> Do you have an idea about where this would go better? On the
g_browser_process?

my guess is that we'd want this in content because a lot of web platform
features go to disk now.

i havent thought about this enough to figure out where it should go. it just
seemed very different from the other BrowserThreads, hence my question

[brettw, 2012-01-03 21:14:09]

Yeah, it just seems to me that BrowserThread is where people look for when they
want to post stuff to other threads.

Currently browser_thread.h is a bit weird and combines two concepts. The first
is the object BrowserThread which is clearly not associated with our worker
pool. The second is the ability to expose all known threads in the browser and
be able to post to them in a safe way. I think the worker pool fits in with this
second category of stuff.

[jam, 2012-01-03 21:30:12]

On 2012/01/03 21:14:09, brettw wrote:
> Yeah, it just seems to me that BrowserThread is where people look for when
they
> want to post stuff to other threads.
> 
> Currently browser_thread.h is a bit weird and combines two concepts. The first
> is the object BrowserThread which is clearly not associated with our worker
> pool. The second is the ability to expose all known threads in the browser and
> be able to post to them in a safe way. I think the worker pool fits in with
this
> second category of stuff.

the reason these two concepts (known IDs and task posting) are combined is that
it makes the implementation efficient (it knows when to not use locks).

regardless of where this goes though, the example conversion in
visitedlink_master worries me a little because it seems like a bit of work to
convert all the places (add member variable, check it first and if null call a
function, switch all the code in a class to call that function). it seems like
too much copy&paste to do if we convert most of our disk access code to use this
(which I think we want to). i don't have enough time right now to think of
better solutions (maybe we can chat in person tomorrow), but what do you think
of this?

[brettw, 2012-01-03 21:36:19]

You can always get a token synchronously or ask for a named one every time. The
visited link master is on the blocking path for browser startup so I wanted to
avoid the extra work (getting the token involves some locking).

[jam, 2012-01-03 22:37:05]

On 2012/01/03 21:36:19, brettw wrote:
> You can always get a token synchronously or ask for a named one every time.
The
> visited link master is on the blocking path for browser startup so I wanted to
> avoid the extra work (getting the token involves some locking).

we do want all users to be as efficient as possible. i.e. i think it's confusing
to developers, and error-prone, to have two different paths, one faster with
locks once, and the other slower but easier to use. note that the implementation
of browserthread was done in a way that no locks were needed (on BT's internal
objects, but not ML of course) to post a task to a thread with a longer lifetime
than the current one.

it seems to me that we want one way to post threads, and we we want it as simple
as possible. that argues for something similar to what currently exists, i.e.
just a single method call (on BT?) that always passes the token string.

[brettw, 2012-01-04 03:56:17]

New snap up with no lazy initialization. Turns out it requires no locking to
generate the token so there's no ,reason not to do it at startup

[brettw, 2012-01-05 05:22:47]

This version adds some helper functions as discussed in person. I used them in
one simple place.

Interesting question (which I would prefer to address in a followup): how can we
assert that you're on the worker pool? Lots of people DCHECK that they're on the
file thread. Maybe set some kind of TLS flag at the start of each worker that we
can check?

[jam, 2012-01-05 18:11:32]

lgtm

On 2012/01/05 05:22:47, brettw wrote:
> This version adds some helper functions as discussed in person. I used them in
> one simple place.
> 
> Interesting question (which I would prefer to address in a followup): how can
we
> assert that you're on the worker pool? Lots of people DCHECK that they're on
the
> file thread. Maybe set some kind of TLS flag at the start of each worker that
we
> can check?

TLS sounds good. an alternative is that the DCHECK can call a method on the
worker pool code to check if the current ML is in its list of threads.