Add SequencedWorkerPool.IsRunningSequenceOnCurrentThread so callers can make stronger assertions ab…

Add SequencedWorkerPool.IsRunningSequenceOnCurrentThread so callers can make stronger assertions about where methods are being called.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129979

[michaeln, 2012-03-23 21:32:37]

https://chromiumcodereview.appspot.com/9845037/diff/6003/base/threading/seque...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://chromiumcodereview.appspot.com/9845037/diff/6003/base/threading/seque...
base/threading/sequenced_worker_pool_unittest.cc:513: // TODO(michaeln): write a
test for IsRunningSequenceOnCurrentThread
i'll add some tests if this method looks like a reasonable addition

[michaeln, 2012-03-24 01:05:41]

when ahead and added the test

[michaeln, 2012-03-25 00:16:20]

Updated to function properly given multiple pools in the same process (updated
tests too), and fixed a serious bug around shutdown in my original CL.

[michaeln, 2012-03-25 00:20:01]

https://chromiumcodereview.appspot.com/9845037/diff/3008/base/threading/seque...
File base/threading/sequenced_worker_pool.cc (right):

https://chromiumcodereview.appspot.com/9845037/diff/3008/base/threading/seque...
base/threading/sequenced_worker_pool.cc:382: bool
SequencedWorkerPool::Inner::RunsTasksOnCurrentThread() const {
we could hoist these method bodies into SequencedWorkerPool directly and remove
these two Inner methods

[michaeln, 2012-03-25 01:03:52]

adding akalin on the review list too since he's done a bunch of work on this
too, a review from either of you on this would be good enough, whomever gets to
it first please remove the other from the review list

[akalin, 2012-03-26 04:28:49]

On 2012/03/25 01:03:52, michaeln wrote:
> adding akalin on the review list too since he's done a bunch of work on this
> too, a review from either of you on this would be good enough, whomever gets
to
> it first please remove the other from the review list

Hmm, I'm not a fan of this change.  It adds a SWP-specific checking method, so
it makes people use SWP directly instead of the TaskRunner interfaces as they
should.  And if you're using SequencedTaskRunner already, you don't really need
this stronger check (assuming it works correctly).

[michaeln, 2012-03-26 18:50:56]

TaskRunner is a nice interface, but the SWP class also stands on its own... i
think this new method adds value to its interface... i know i'd make use of it
in the DomStorage library. In addition to DCHECKING control flow, another common
use is to conditionally PostTask to a particular target if execution is not
running on that target. There is no way to do that if the 'target' is a
particular sequence within a SWP. This method provides a way to do that.

Right now, there is no general purpose implementation of SequencedTaskRunner
that posts tasks to SWP in the code base. If/when there is, a decent way to
implement the RunsTasksOnCurrentThread() method of that concrete class would be
to call this new method. That would allow both forms of usage (correctness
DCHECKs and conditionally PostTask).

[michaeln, 2012-03-27 01:46:26]

If we go with TLS usage for this, i think the threads_ collection could be
reverted back to it's simpler form...
std::vector<linked_ptr<Worker> > threads_;

[akalin, 2012-03-27 04:07:27]

On 2012/03/27 01:46:26, michaeln wrote:
> If we go with TLS usage for this, i think the threads_ collection could be
> reverted back to it's simpler form...
> std::vector<linked_ptr<Worker> > threads_;

Yeah, but the TLS usage is precisely what I want to avoid. :-) I think a better
approach would be to do something similar to the current implementation of
RunsTasksOnCurrentThread(), and look up the worker pointer via the current
thread id, and then check the current sequence token against it.

It seems easier to reason about to me.  Lock contention doesn't seem like an
issue, given that the primary use case is debugging.

[michaeln, 2012-03-27 04:29:00]

> Yeah, but the TLS usage is precisely what I want to avoid. :-) I think a
better
> approach would be to do something similar to the current implementation of
> RunsTasksOnCurrentThread(), and look up the worker pointer via the current
> thread id, and then check the current sequence token against it.
> 
> It seems easier to reason about to me.  Lock contention doesn't seem like an
> issue, given that the primary use case is debugging.

Certainly could go back to looking up by tid, but not sure thats better really.

As you noted, there would be additional lock contention with doing so. It's not
clear the primary use case will be debugging, many consumers of
IsCurrentlyRunningOn stuff today use it for conditional posting purposes. And
given what this class does, it's pretty justifiable that to have it consume a
TLS slot.

And for me at least, TLS is actually easier to reason about (fewer moving
parts:) Aside from that grokkage point, is there another reason to avoid it?

[akalin, 2012-03-27 05:37:02]

On 2012/03/27 04:29:00, michaeln wrote:
> Certainly could go back to looking up by tid, but not sure thats better
really.
> 
> As you noted, there would be additional lock contention with doing so. It's
not
> clear the primary use case will be debugging, many consumers of
> IsCurrentlyRunningOn stuff today use it for conditional posting purposes. And
> given what this class does, it's pretty justifiable that to have it consume a
> TLS slot.
>
> And for me at least, TLS is actually easier to reason about (fewer moving
> parts:) Aside from that grokkage point, is there another reason to avoid it?

Some data to support my assertion:

$ git grep 'BelongsToCurrentThread()' | wc -l
     470
$ git grep 'DCHECK.*BelongsToCurrentThread()' | wc -l 
     348
$ git grep 'if.*BelongsToCurrentThread()' | wc -l
      88

TLS is 'easier to reason about' in the same way that a global is, and they're
both problematic in similar ways.

Even the comment at the top of thread_local.h supports my position:

// WARNING: Thread local storage is a bit tricky to get right.  Please make
// sure that this is really the proper solution for what you're trying to
// achieve.  Don't prematurely optimize, most likely you can just use a Lock.

[michaeln, 2012-03-27 20:11:13]

The ratio of usage aside, in absolute terms there are a lot of callsites. There
are also thousands (!) of uses of BrowserThread::CurrentlyOn(x) which is very
similar in nature and worth noting since we should expect that SWP will replace
some amount of that usage over time.

I think all that adds up to we should expect this method to be used a fair
amount, with something around a 4 to 1 ratio of debug to release. There's enough
usage to warrant some thought.

fwiw, I wrote it in terms of tls because that's what felt most natural to me
(part of that determination of what felt "natrual" was that it avoided
unnecessary locking). I think it's mostly a wash on lock+map vs tls in terms
easy to grok'ness (both are easy enough), and where an individual lands on that
point is pretty subjective.

Something that isn't subjective is the runtime costs associated with the extra
locking. I ran some tests with locking vs w/o locking in the IsRunningOn
methods.

* WorkerPool configured with 3 workers.
* Release build.
* Four runs with each setup.
* Each task calls the IsRunningOn method once.

Below are the number of tasks run in a 10 second window on each worker in each
run. There's a noticible bump up for the no lock case, but it's not huge.

// NO LOCK

  Actual: 653687
  Actual: 646548
  Actual: 654791

  Actual: 646746
  Actual: 648784
  Actual: 640562

  Actual: 685038
  Actual: 674428
  Actual: 688140

  Actual: 622904
  Actual: 632406
  Actual: 627606

// WITH LOCK

  Actual: 563205
  Actual: 560909
  Actual: 563811

  Actual: 622546
  Actual: 621420
  Actual: 625818

  Actual: 559578
  Actual: 561536
  Actual: 569366

  Actual: 590046
  Actual: 599915
  Actual: 596212

I can go either way since the numbers aren't all that different, but for me at
least, the tls way looks slightly better on all dimensions, so that's my
preference.

[akalin, 2012-03-27 20:30:19]

On 2012/03/27 20:11:13, michaeln wrote:
> Below are the number of tasks run in a 10 second window on each worker in each
> run. There's a noticible bump up for the no lock case, but it's not huge.
...
> I can go either way since the numbers aren't all that different, but for me at
> least, the tls way looks slightly better on all dimensions, so that's my
> preference.

I'm not denying that the lock has a perf hit, but when we're talking about 50k
tasks/sec + 60k tasks/sec, I think that puts this in the realm of
micro-optimizing.

To make forward progress, why don't you move this review over to brett? (I'm
leaving for vacation tomorrow anyway.) I'll defer to his decision, although as
(inadvertent) de facto maintainer of SWP my (pretty strong) vote would be for
avoiding the thread-local var.

[michaeln, 2012-03-27 21:26:47]

> To make forward progress, why don't you move this review over to brett? (I'm
> leaving for vacation tomorrow anyway.) I'll defer to his decision, although as
> (inadvertent) de facto maintainer of SWP my (pretty strong) vote would be for
> avoiding the thread-local var.

I'd be happy to get something in rather than drag it out, and since you have a
pretty strong preference one way while i have a slight preference the other, i
can switch it over even though i dont fully agree/understand the basis of your
strong preference. I see now that i should get an OWNER review from brett too, i
wish he had chimed in by now?

[michaeln, 2012-03-27 22:16:40]

switched to lock + map

[michaeln, 2012-03-28 19:01:40]

ping

[akalin, 2012-03-28 20:24:09]

LGTM after nits, thanks

http://codereview.chromium.org/9845037/diff/22001/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool_unittest.cc (right):

http://codereview.chromium.org/9845037/diff/22001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:513: static void
IsRunningOnCurrentThreadTask(
nit: no static needed since you're already in anon namespace

http://codereview.chromium.org/9845037/diff/22001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:516: SequencedWorkerPool* pool,
any particular reason this takes a raw pointer instead of a refptr?  if not, I'd
prefer the scoped_refptr

http://codereview.chromium.org/9845037/diff/22001/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool_unittest.cc:528:
TEST_F(SequencedWorkerPoolTest, IsRunningOnCurrentThread) {
add test-level comment explaining what the test is doing

[brettw, 2012-03-28 20:26:37]

LGTM rubberstamp based on Fred's review.

[michaeln, 2012-03-30 22:35:28]

https://chromiumcodereview.appspot.com/9845037/diff/22001/base/threading/sequ...
File base/threading/sequenced_worker_pool_unittest.cc (right):

https://chromiumcodereview.appspot.com/9845037/diff/22001/base/threading/sequ...
base/threading/sequenced_worker_pool_unittest.cc:513: static void
IsRunningOnCurrentThreadTask(
On 2012/03/28 20:24:09, akalin wrote:
> nit: no static needed since you're already in anon namespace

Done.

https://chromiumcodereview.appspot.com/9845037/diff/22001/base/threading/sequ...
base/threading/sequenced_worker_pool_unittest.cc:516: SequencedWorkerPool* pool,
On 2012/03/28 20:24:09, akalin wrote:
> any particular reason this takes a raw pointer instead of a refptr?  if not,
I'd
> prefer the scoped_refptr

I updated the base::Bind callsites to not use Unretained(), more readable. Also
now the closure's internal storage constains scoped_refptr<>s to these guys to
ensure these ptrs are valid. Passing scoped_refptr<> by value serves no purpose
at runtime (just churns the refcount), passing const scoped_repftr<>& is
somewhat confusing (what's const exactly), and both decrease readability.

https://chromiumcodereview.appspot.com/9845037/diff/22001/base/threading/sequ...
base/threading/sequenced_worker_pool_unittest.cc:528:
TEST_F(SequencedWorkerPoolTest, IsRunningOnCurrentThread) {
On 2012/03/28 20:24:09, akalin wrote:
> add test-level comment explaining what the test is doing

Done.