base: Change WeakPtr to use SequenceChecker instead of ThreadChecker.

base: Change WeakPtr to use SequenceChecker instead of ThreadChecker.

This will enable WeakPtr to be used in SequencedWorkerPool, et al. with a sequence token.

BUG=165590

[tommycli, 2013-06-28 20:17:31]

vandebo:

Can I get an initial sanity check on my testing strategy? I basically threw out
the existing test because it was based on the old design.

I based the new test on thread_checker_unittest.h. The test currently copies
many of the tests from thread_checker_unittest.h, since we expect
SequenceChecker to behave exactly the same way in the absence of a
SequencedWorkerPool.

SequenceCheckerTest also uses death tests just like ThreadCheckerTest does to
test the invalid usage cases in both DEBUG and RELEASE.

I go to lunch, we can talk after...

[vandebo (ex-Chrome), 2013-06-28 21:00:06]

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h
File base/sequence_checker.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h#ne...
base/sequence_checker.h:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
nit: current policy is to not update copyright notices.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h#ne...
base/sequence_checker.h:35: 
DetachFromSequence() missing?

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl.cc
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.cc:46: // If can't get sequence token, lock to the
thread ID.
If you got a pool, probably should DCHECK that you get a token.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.h:35: protected:
why not private?

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.h:45: mutable SequencedWorkerPool* associated_pool_;
nit: pool_

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
base/sequence_checker_unittest.cc:49: // Calls ThreadCheckerClass::DoStuff on
another thread.
nit: this doesn't do it on a different thread, but is a helper to enable that...

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
base/sequence_checker_unittest.cc:81: scoped_ptr<SequenceCheckerClass>
sequence_checker_class_;
nit: why is this one a scope_ptr, but CallDoStuffOnThread has a raw ptr?

https://codereview.chromium.org/18231002/diff/8001/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool* Get();
nit: Current() is a more traditional name for this method.

[tommycli, 2013-06-28 22:37:07]

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h
File base/sequence_checker.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h#ne...
base/sequence_checker.h:1: // Copyright 2013 The Chromium Authors. All rights
reserved.
On 2013/06/28 21:00:06, vandebo wrote:
> nit: current policy is to not update copyright notices.

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker.h#ne...
base/sequence_checker.h:35: 
On 2013/06/28 21:00:06, vandebo wrote:
> DetachFromSequence() missing?

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl.cc
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.cc:46: // If can't get sequence token, lock to the
thread ID.
On 2013/06/28 21:00:06, vandebo wrote:
> If you got a pool, probably should DCHECK that you get a token.

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl.h
File base/sequence_checker_impl.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.h:35: protected:
On 2013/06/28 21:00:06, vandebo wrote:
> why not private?

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_impl...
base/sequence_checker_impl.h:45: mutable SequencedWorkerPool* associated_pool_;
On 2013/06/28 21:00:06, vandebo wrote:
> nit: pool_

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
base/sequence_checker_unittest.cc:49: // Calls ThreadCheckerClass::DoStuff on
another thread.
On 2013/06/28 21:00:06, vandebo wrote:
> nit: this doesn't do it on a different thread, but is a helper to enable
that...

Done.

https://codereview.chromium.org/18231002/diff/8001/base/sequence_checker_unit...
base/sequence_checker_unittest.cc:81: scoped_ptr<SequenceCheckerClass>
sequence_checker_class_;
On 2013/06/28 21:00:06, vandebo wrote:
> nit: why is this one a scope_ptr, but CallDoStuffOnThread has a raw ptr?

DeleteThreadCheckerClassThread must take possession of the object to delete it.
CallDoStuffThread should not take possession.

https://codereview.chromium.org/18231002/diff/8001/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18231002/diff/8001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool* Get();
On 2013/06/28 21:00:06, vandebo wrote:
> nit: Current() is a more traditional name for this method.

Done.

[tommycli, 2013-06-28 22:51:07]

akalin: I changed your SequenceChecker to fit it into WeakPtr's use pattern. I'd
like your opinion. Thanks!

[akalin, 2013-06-28 23:32:00]

https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
Current();
i don't really like exposing this. seems better to me to just expose the
sequence token. Can't we use the thread id name manager?

[tommycli, 2013-06-28 23:49:49]

https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
Current();
On 2013/06/28 23:32:00, akalin wrote:
> i don't really like exposing this. seems better to me to just expose the
> sequence token. Can't we use the thread id name manager?

Since SequencedWorkerPool is not enforced to be a singleton, there could
theoretically be two different threads owned by two different
SequenceWorkerPools with the same SequenceToken. What do you think?

[akalin, 2013-06-28 23:51:16]

On 2013/06/28 23:49:49, tommycli wrote:
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> File base/threading/sequenced_worker_pool.h (right):
> 
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
> Current();
> On 2013/06/28 23:32:00, akalin wrote:
> > i don't really like exposing this. seems better to me to just expose the
> > sequence token. Can't we use the thread id name manager?
> 
> Since SequencedWorkerPool is not enforced to be a singleton, there could
> theoretically be two different threads owned by two different
> SequenceWorkerPools with the same SequenceToken. What do you think?

That seems difficult to do, since the worker threads are visible only by the SWP
that created it. It's possible for a worker thread to have its own SWP I guess,
but then its worker threads would still be distinct.

[tommycli, 2013-06-29 00:09:35]

On 2013/06/28 23:51:16, akalin wrote:
> On 2013/06/28 23:49:49, tommycli wrote:
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > File base/threading/sequenced_worker_pool.h (right):
> > 
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
> > Current();
> > On 2013/06/28 23:32:00, akalin wrote:
> > > i don't really like exposing this. seems better to me to just expose the
> > > sequence token. Can't we use the thread id name manager?
> > 
> > Since SequencedWorkerPool is not enforced to be a singleton, there could
> > theoretically be two different threads owned by two different
> > SequenceWorkerPools with the same SequenceToken. What do you think?
> 
> That seems difficult to do, since the worker threads are visible only by the
SWP
> that created it. It's possible for a worker thread to have its own SWP I
guess,
> but then its worker threads would still be distinct.

If you make two SWPs, and request one token from each, you'll get a
SequenceToken(1) from both. Then if you post a task to each of them with their
respective tokens, you'll have equal SequenceTokens.

I admit this is a total edge case, but it's technically allowed by the SWP API.

[akalin, 2013-06-29 00:33:11]

On 2013/06/29 00:09:35, tommycli wrote:
> On 2013/06/28 23:51:16, akalin wrote:
> > On 2013/06/28 23:49:49, tommycli wrote:
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > File base/threading/sequenced_worker_pool.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
> > > Current();
> > > On 2013/06/28 23:32:00, akalin wrote:
> > > > i don't really like exposing this. seems better to me to just expose the
> > > > sequence token. Can't we use the thread id name manager?
> > > 
> > > Since SequencedWorkerPool is not enforced to be a singleton, there could
> > > theoretically be two different threads owned by two different
> > > SequenceWorkerPools with the same SequenceToken. What do you think?
> > 
> > That seems difficult to do, since the worker threads are visible only by the
> SWP
> > that created it. It's possible for a worker thread to have its own SWP I
> guess,
> > but then its worker threads would still be distinct.
> 
> If you make two SWPs, and request one token from each, you'll get a
> SequenceToken(1) from both. Then if you post a task to each of them with their
> respective tokens, you'll have equal SequenceTokens.
> 
> I admit this is a total edge case, but it's technically allowed by the SWP
API.

Ah, I see what you mean now.

What I'm concerned about is random stuff being able to poke at the owner SWP. If
we store an opaque token in the TLS variable, I'm okay with that. That opaque
token may well be <static_cast<uintptr_t>(pointer_to_SWP), sequence_token>. Make
sense?

[tommycli, 2013-06-29 04:02:22]

On 2013/06/29 00:33:11, akalin wrote:
> On 2013/06/29 00:09:35, tommycli wrote:
> > On 2013/06/28 23:51:16, akalin wrote:
> > > On 2013/06/28 23:49:49, tommycli wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > File base/threading/sequenced_worker_pool.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > base/threading/sequenced_worker_pool.h:147: static SequencedWorkerPool*
> > > > Current();
> > > > On 2013/06/28 23:32:00, akalin wrote:
> > > > > i don't really like exposing this. seems better to me to just expose
the
> > > > > sequence token. Can't we use the thread id name manager?
> > > > 
> > > > Since SequencedWorkerPool is not enforced to be a singleton, there could
> > > > theoretically be two different threads owned by two different
> > > > SequenceWorkerPools with the same SequenceToken. What do you think?
> > > 
> > > That seems difficult to do, since the worker threads are visible only by
the
> > SWP
> > > that created it. It's possible for a worker thread to have its own SWP I
> > guess,
> > > but then its worker threads would still be distinct.
> > 
> > If you make two SWPs, and request one token from each, you'll get a
> > SequenceToken(1) from both. Then if you post a task to each of them with
their
> > respective tokens, you'll have equal SequenceTokens.
> > 
> > I admit this is a total edge case, but it's technically allowed by the SWP
> API.
> 
> Ah, I see what you mean now.
> 
> What I'm concerned about is random stuff being able to poke at the owner SWP.
If
> we store an opaque token in the TLS variable, I'm okay with that. That opaque
> token may well be <static_cast<uintptr_t>(pointer_to_SWP), sequence_token>.
Make
> sense?

That sounds like a good idea. I will implement that on Monday.

[tommycli, 2013-07-01 17:23:37]

On 2013/06/29 04:02:22, tommycli wrote:
> On 2013/06/29 00:33:11, akalin wrote:
> > On 2013/06/29 00:09:35, tommycli wrote:
> > > On 2013/06/28 23:51:16, akalin wrote:
> > > > On 2013/06/28 23:49:49, tommycli wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > File base/threading/sequenced_worker_pool.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > base/threading/sequenced_worker_pool.h:147: static
SequencedWorkerPool*
> > > > > Current();
> > > > > On 2013/06/28 23:32:00, akalin wrote:
> > > > > > i don't really like exposing this. seems better to me to just expose
> the
> > > > > > sequence token. Can't we use the thread id name manager?
> > > > > 
> > > > > Since SequencedWorkerPool is not enforced to be a singleton, there
could
> > > > > theoretically be two different threads owned by two different
> > > > > SequenceWorkerPools with the same SequenceToken. What do you think?
> > > > 
> > > > That seems difficult to do, since the worker threads are visible only by
> the
> > > SWP
> > > > that created it. It's possible for a worker thread to have its own SWP I
> > > guess,
> > > > but then its worker threads would still be distinct.
> > > 
> > > If you make two SWPs, and request one token from each, you'll get a
> > > SequenceToken(1) from both. Then if you post a task to each of them with
> their
> > > respective tokens, you'll have equal SequenceTokens.
> > > 
> > > I admit this is a total edge case, but it's technically allowed by the SWP
> > API.
> > 
> > Ah, I see what you mean now.
> > 
> > What I'm concerned about is random stuff being able to poke at the owner
SWP.
> If
> > we store an opaque token in the TLS variable, I'm okay with that. That
opaque
> > token may well be <static_cast<uintptr_t>(pointer_to_SWP), sequence_token>.
> Make
> > sense?
> 
> That sounds like a good idea. I will implement that on Monday.

Hi Fred - how do you feel about making the Current() method that retrieves the
SWP 'private', and having SequenceChecker be the only 'friend'ed class?

I ask for two reasons:
 - Our current thread_local.h only supports pointers and booleans.
 - Storing an opaque token would require updating the token every time we start
a new task on the SWP. (Not a huge overhead, but some overhead anyways)

What do you think?

Tommy

[akalin, 2013-07-01 17:44:44]

On 2013/07/01 17:23:37, tommycli wrote:
> On 2013/06/29 04:02:22, tommycli wrote:
> > On 2013/06/29 00:33:11, akalin wrote:
> > > On 2013/06/29 00:09:35, tommycli wrote:
> > > > On 2013/06/28 23:51:16, akalin wrote:
> > > > > On 2013/06/28 23:49:49, tommycli wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > > File base/threading/sequenced_worker_pool.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > > base/threading/sequenced_worker_pool.h:147: static
> SequencedWorkerPool*
> > > > > > Current();
> > > > > > On 2013/06/28 23:32:00, akalin wrote:
> > > > > > > i don't really like exposing this. seems better to me to just
expose
> > the
> > > > > > > sequence token. Can't we use the thread id name manager?
> > > > > > 
> > > > > > Since SequencedWorkerPool is not enforced to be a singleton, there
> could
> > > > > > theoretically be two different threads owned by two different
> > > > > > SequenceWorkerPools with the same SequenceToken. What do you think?
> > > > > 
> > > > > That seems difficult to do, since the worker threads are visible only
by
> > the
> > > > SWP
> > > > > that created it. It's possible for a worker thread to have its own SWP
I
> > > > guess,
> > > > > but then its worker threads would still be distinct.
> > > > 
> > > > If you make two SWPs, and request one token from each, you'll get a
> > > > SequenceToken(1) from both. Then if you post a task to each of them with
> > their
> > > > respective tokens, you'll have equal SequenceTokens.
> > > > 
> > > > I admit this is a total edge case, but it's technically allowed by the
SWP
> > > API.
> > > 
> > > Ah, I see what you mean now.
> > > 
> > > What I'm concerned about is random stuff being able to poke at the owner
> SWP.
> > If
> > > we store an opaque token in the TLS variable, I'm okay with that. That
> opaque
> > > token may well be <static_cast<uintptr_t>(pointer_to_SWP),
sequence_token>.
> > Make
> > > sense?
> > 
> > That sounds like a good idea. I will implement that on Monday.
> 
> Hi Fred - how do you feel about making the Current() method that retrieves the
> SWP 'private', and having SequenceChecker be the only 'friend'ed class?
> 
> I ask for two reasons:
>  - Our current thread_local.h only supports pointers and booleans.
>  - Storing an opaque token would require updating the token every time we
start
> a new task on the SWP. (Not a huge overhead, but some overhead anyways)
> 
> What do you think?
> 
> Tommy

Okay, that sounds good.

Also, I think a better name would be "Owner()" or something, since it's non-NULL
only for worker threads.

[tommycli, 2013-07-01 18:07:38]

On 2013/07/01 17:44:44, akalin wrote:
> On 2013/07/01 17:23:37, tommycli wrote:
> > On 2013/06/29 04:02:22, tommycli wrote:
> > > On 2013/06/29 00:33:11, akalin wrote:
> > > > On 2013/06/29 00:09:35, tommycli wrote:
> > > > > On 2013/06/28 23:51:16, akalin wrote:
> > > > > > On 2013/06/28 23:49:49, tommycli wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > > > File base/threading/sequenced_worker_pool.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/18231002/diff/33001/base/threading/sequenced_...
> > > > > > > base/threading/sequenced_worker_pool.h:147: static
> > SequencedWorkerPool*
> > > > > > > Current();
> > > > > > > On 2013/06/28 23:32:00, akalin wrote:
> > > > > > > > i don't really like exposing this. seems better to me to just
> expose
> > > the
> > > > > > > > sequence token. Can't we use the thread id name manager?
> > > > > > > 
> > > > > > > Since SequencedWorkerPool is not enforced to be a singleton, there
> > could
> > > > > > > theoretically be two different threads owned by two different
> > > > > > > SequenceWorkerPools with the same SequenceToken. What do you
think?
> > > > > > 
> > > > > > That seems difficult to do, since the worker threads are visible
only
> by
> > > the
> > > > > SWP
> > > > > > that created it. It's possible for a worker thread to have its own
SWP
> I
> > > > > guess,
> > > > > > but then its worker threads would still be distinct.
> > > > > 
> > > > > If you make two SWPs, and request one token from each, you'll get a
> > > > > SequenceToken(1) from both. Then if you post a task to each of them
with
> > > their
> > > > > respective tokens, you'll have equal SequenceTokens.
> > > > > 
> > > > > I admit this is a total edge case, but it's technically allowed by the
> SWP
> > > > API.
> > > > 
> > > > Ah, I see what you mean now.
> > > > 
> > > > What I'm concerned about is random stuff being able to poke at the owner
> > SWP.
> > > If
> > > > we store an opaque token in the TLS variable, I'm okay with that. That
> > opaque
> > > > token may well be <static_cast<uintptr_t>(pointer_to_SWP),
> sequence_token>.
> > > Make
> > > > sense?
> > > 
> > > That sounds like a good idea. I will implement that on Monday.
> > 
> > Hi Fred - how do you feel about making the Current() method that retrieves
the
> > SWP 'private', and having SequenceChecker be the only 'friend'ed class?
> > 
> > I ask for two reasons:
> >  - Our current thread_local.h only supports pointers and booleans.
> >  - Storing an opaque token would require updating the token every time we
> start
> > a new task on the SWP. (Not a huge overhead, but some overhead anyways)
> > 
> > What do you think?
> > 
> > Tommy
> 
> Okay, that sounds good.
> 
> Also, I think a better name would be "Owner()" or something, since it's
non-NULL
> only for worker threads.

Sounds good. I made it 'private' and named 'Owner()'. I'm still fighting some
IWYU issues to make it compile on every platform, but let me know what you
think.

[tommycli, 2013-07-01 21:25:17]

darin: Ready for a base/ OWNER review. Been working on this with akalin and
vandebo.

[darin (slow to review), 2013-07-02 13:54:23]

This looks pretty good.  I just have one substantive comment.

https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:23: if (!pool_)
this probably deserves a comment to draw attention to behaving like
ThreadChecker.  i realize you already document this in the header, so just a
brief note here might be enough.

https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.cc:563: ThreadMap::const_iterator found =
threads_.find(PlatformThread::CurrentId());
Why do we need TLS to find the owner and this map indexed by thread_id to find
the token?  It seems like one or the other should be sufficient.

Perhaps threads_ could just be owned by a singleton?  Then you could have a
static function to query it?

[tommycli, 2013-07-02 17:22:57]

darin: addressed your comments below

https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_imp...
File base/sequence_checker_impl.cc (right):

https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_imp...
base/sequence_checker_impl.cc:23: if (!pool_)
On 2013/07/02 13:54:23, darin wrote:
> this probably deserves a comment to draw attention to behaving like
> ThreadChecker.  i realize you already document this in the header, so just a
> brief note here might be enough.

Done.

https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.cc:563: ThreadMap::const_iterator found =
threads_.find(PlatformThread::CurrentId());
On 2013/07/02 13:54:23, darin wrote:
> Why do we need TLS to find the owner and this map indexed by thread_id to find
> the token?  It seems like one or the other should be sufficient.
> 
> Perhaps threads_ could just be owned by a singleton?  Then you could have a
> static function to query it?

Sequence tokens are assigned from positive integers 1, 2, 3, etc. and each SWP
starts counting from 1. If you make two SWPs, they can issue equal
SequenceTokens.

You need to query the owner of the thread so you can distinguish between <SWP-1,
SequenceToken(1)> and <SWP-2, SequenceToken(1)>.

Alternative solutions were: 
1. SequencedWorkerPool was enforced to be a singleton, 
2. Add extra member to SequenceToken to store a pointer to the issuing SWP.

Neither seemed all that appealing...

[darin (slow to review), 2013-07-02 21:41:51]

I see.  It seems like the sequence token assigner could be the singleton
too.  Had you considered that?
On Jul 2, 2013 10:22 AM, <tommycli@chromium.org> wrote:

> darin: addressed your comments below
>
>
> https://codereview.chromium.**org/18231002/diff/73001/base/**
>
sequence_checker_impl.cc<https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_impl.cc>
> File base/sequence_checker_impl.cc (right):
>
> https://codereview.chromium.**org/18231002/diff/73001/base/**
>
sequence_checker_impl.cc#**newcode23<https://codereview.chromium.org/18231002/diff/73001/base/sequence_checker_impl.cc#newcode23>
> base/sequence_checker_impl.cc:**23: if (!pool_)
> On 2013/07/02 13:54:23, darin wrote:
>
>> this probably deserves a comment to draw attention to behaving like
>> ThreadChecker.  i realize you already document this in the header, so
>>
> just a
>
>> brief note here might be enough.
>>
>
> Done.
>
> https://codereview.chromium.**org/18231002/diff/73001/base/**
>
threading/sequenced_worker_**pool.cc<https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_worker_pool.cc>
> File base/threading/sequenced_**worker_pool.cc (right):
>
> https://codereview.chromium.**org/18231002/diff/73001/base/**
>
threading/sequenced_worker_**pool.cc#newcode563<https://codereview.chromium.org/18231002/diff/73001/base/threading/sequenced_worker_pool.cc#newcode563>
> base/threading/sequenced_**worker_pool.cc:563: ThreadMap::const_iterator
> found = threads_.find(PlatformThread::**CurrentId());
> On 2013/07/02 13:54:23, darin wrote:
>
>> Why do we need TLS to find the owner and this map indexed by thread_id
>>
> to find
>
>> the token?  It seems like one or the other should be sufficient.
>>
>
>  Perhaps threads_ could just be owned by a singleton?  Then you could
>>
> have a
>
>> static function to query it?
>>
>
> Sequence tokens are assigned from positive integers 1, 2, 3, etc. and
> each SWP starts counting from 1. If you make two SWPs, they can issue
> equal SequenceTokens.
>
> You need to query the owner of the thread so you can distinguish between
> <SWP-1, SequenceToken(1)> and <SWP-2, SequenceToken(1)>.
>
> Alternative solutions were:
> 1. SequencedWorkerPool was enforced to be a singleton,
> 2. Add extra member to SequenceToken to store a pointer to the issuing
> SWP.
>
> Neither seemed all that appealing...
>
>
https://codereview.chromium.**org/18231002/<https://codereview.chromium.org/1...
>

[tommycli, 2013-07-03 00:45:32]

darin: Here's a version that assigns SequenceTokens that are unique across all
SWP instances. Let me know if you like this any better.

[darin (slow to review), 2013-07-03 07:59:41]

https://codereview.chromium.org/18231002/diff/122001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/18231002/diff/122001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:396: static volatile subtle::Atomic32
g_last_sequence_number_;
Looks like this code might benefit from using base::StaticAtomicSequenceNumber
(see base/atomic_sequence_num.h)

[tommycli, 2013-07-03 18:16:30]

darin: Patchset 23 uses StaticAtomicSequenceNumber.

https://codereview.chromium.org/18231002/diff/122001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/18231002/diff/122001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:396: static volatile subtle::Atomic32
g_last_sequence_number_;
On 2013/07/03 07:59:41, darin wrote:
> Looks like this code might benefit from using base::StaticAtomicSequenceNumber
> (see base/atomic_sequence_num.h)

Done.

[tommycli, 2013-07-03 18:35:44]

This CL is closed due to reitveld bug.

Continued here: https://chromiumcodereview.appspot.com/18501008/