base: Make SequencedWorkerPool issue globally unique SequenceTokens.

base: Make SequencedWorkerPool issue globally unique SequenceTokens.

SequencedWorkerPool currently issues SequenceTokens out of an internal member counter. This means that two different SequencedWorkerPool instances can issue identical SequenceTokens, which mucks up any attempt to distinguish sequences using only SequenceTokens.

This change makes the SequenceTokens issued from an StaticAtomicSequenceNumber, which is globally shared amongst all SequencedWorkerPools.

This change also makes the SequencedWorkerPool included in the nacl_untrusted builds, as it is needed for SequenceChecker and WeakPtr to work correctly. It previously was excluded because it used base/metrics. I've #ifdefed the base/metrics usage out for nacl.

This issue is a spinoff and pre-requisite of issue 18501008: Make WeakPtr use SequenceChecker instead of ThreadChecker.

R=akalin,darin
BUG=165590
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210423

[tommycli, 2013-07-03 20:21:28]

akalin/darin: At akalin's request, this is an issue with the SequencedWorkerPool
changes broken out. This is a pre-req to the other issue now.

[akalin, 2013-07-03 20:41:14]

https://codereview.chromium.org/18650006/diff/1/base/base.gypi
File base/base.gypi (left):

https://codereview.chromium.org/18650006/diff/1/base/base.gypi#oldcode704
base/base.gypi:704: 'threading/sequenced_worker_pool.cc',
can you mention the nacl-specific changes in the CL description?

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
base/threading/sequenced_worker_pool.h:149: static bool
CurrentThreadSequenceToken(SequenceToken* result_token);
valid sequence tokens are non-zero. can you have this return a SequenceToken
that is zero on failure instead?

[tommycli, 2013-07-03 21:04:02]

https://codereview.chromium.org/18650006/diff/1/base/base.gypi
File base/base.gypi (left):

https://codereview.chromium.org/18650006/diff/1/base/base.gypi#oldcode704
base/base.gypi:704: 'threading/sequenced_worker_pool.cc',
On 2013/07/03 20:41:14, akalin wrote:
> can you mention the nacl-specific changes in the CL description?

Done.

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
base/threading/sequenced_worker_pool.h:149: static bool
CurrentThreadSequenceToken(SequenceToken* result_token);
On 2013/07/03 20:41:14, akalin wrote:
> valid sequence tokens are non-zero. can you have this return a SequenceToken
> that is zero on failure instead?

This is kind of a messed-up point about SequencedWorkerPool.

SequenceToken(0) is used internally to indicate an unsequenced task that can be
run at any time. All the SequenceTokens issued by GetSequenceToken are non-zero,
but SequenceToken == 0 isn't quite invalid so much as an internal sentinel
value.

Although I could re-use SequenceToken(0) as an error sentinel value here, I felt
it was bad to assign yet another special meaning to '0'.

If you think this explanation belongs in the code itself, let me know where, and
I'll put it in!

[akalin, 2013-07-03 22:43:46]

D'oh, forgot to send this out earlier.

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
base/threading/sequenced_worker_pool.h:149: static bool
CurrentThreadSequenceToken(SequenceToken* result_token);
On 2013/07/03 21:04:02, tommycli wrote:
> On 2013/07/03 20:41:14, akalin wrote:
> > valid sequence tokens are non-zero. can you have this return a SequenceToken
> > that is zero on failure instead?
> 
> This is kind of a messed-up point about SequencedWorkerPool.
> 
> SequenceToken(0) is used internally to indicate an unsequenced task that can
be
> run at any time. All the SequenceTokens issued by GetSequenceToken are
non-zero,
> but SequenceToken == 0 isn't quite invalid so much as an internal sentinel
> value.
> 
> Although I could re-use SequenceToken(0) as an error sentinel value here, I
felt
> it was bad to assign yet another special meaning to '0'.
> 
> If you think this explanation belongs in the code itself, let me know where,
and
> I'll put it in!

Hmm. Is there a need to distinguish the case where the current thread isn't a
worker thread from the case where the current thread is a worker thread running
an unsequenced task? If not, then I'm still inclined to have it return 0, and
comment that that means that the current thread is either not a worker thread or
is running an unsequenced task. What do you think?

[tommycli, 2013-07-03 23:23:12]

akalin: changed the code to address your last comment.

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/18650006/diff/1/base/threading/sequenced_work...
base/threading/sequenced_worker_pool.h:149: static bool
CurrentThreadSequenceToken(SequenceToken* result_token);
On 2013/07/03 22:43:46, akalin wrote:
> On 2013/07/03 21:04:02, tommycli wrote:
> > On 2013/07/03 20:41:14, akalin wrote:
> > > valid sequence tokens are non-zero. can you have this return a
SequenceToken
> > > that is zero on failure instead?
> > 
> > This is kind of a messed-up point about SequencedWorkerPool.
> > 
> > SequenceToken(0) is used internally to indicate an unsequenced task that can
> be
> > run at any time. All the SequenceTokens issued by GetSequenceToken are
> non-zero,
> > but SequenceToken == 0 isn't quite invalid so much as an internal sentinel
> > value.
> > 
> > Although I could re-use SequenceToken(0) as an error sentinel value here, I
> felt
> > it was bad to assign yet another special meaning to '0'.
> > 
> > If you think this explanation belongs in the code itself, let me know where,
> and
> > I'll put it in!
> 
> Hmm. Is there a need to distinguish the case where the current thread isn't a
> worker thread from the case where the current thread is a worker thread
running
> an unsequenced task? If not, then I'm still inclined to have it return 0, and
> comment that that means that the current thread is either not a worker thread
or
> is running an unsequenced task. What do you think?

Either way seems fine since they both require looking at the comment to figure
out how to use it. I can see the benefits of this approach.

[akalin, 2013-07-03 23:50:12]

lgtm

jar might be a better owners reviewer for this, as he's quite familiar with SWP.
OTOH, he gets busy sometimes. Up to you.

[tommycli, 2013-07-03 23:58:29]

akalin: Thanks for the lg. I'll ask jar.

jar: akalin recommended you as OWNER reviewer for this. This change is a pre-req
to getting WeakPtr to work well with SequencedWorkerPools.

[tommycli, 2013-07-03 23:59:16]

darin: Or if you have time darin, that would be cool too.

[darin (slow to review), 2013-07-05 05:24:19]

LGTM, just some nits:

https://codereview.chromium.org/18650006/diff/7001/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/18650006/diff/7001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.cc:222: SequencedWorkerPool::SequenceToken>
> lazy_tls_ptr =
nit: with the static member variable, you used the "g_" prefix, but with the
file static, you did not.  was that intended?  shouldn't the file static use
"g_" too?

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

https://codereview.chromium.org/18650006/diff/7001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.h:129: // An zero |id_| is a sentinel value
internally for unsequenced tasks.
nit: An -> A, and I think you also meant to put "used" after "value"

That said, this comment seems a bit odd on a public method as consumers probably
don't need to know about implementation details.  What do unsequenced tasks have
to do with a SequenceToken, which is all about sequential-izing tasks?   Who is
expected to call this method?

I think this is about supporting the case where the current thread is executing
an unsequenced task dispatched through a sequenced worker pool.  In other words,
there is no sequence token for such a thread.  The comment for IsValid should
probably just state something like this.

https://codereview.chromium.org/18650006/diff/7001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.h:154: static SequenceToken
CurrentThreadSequenceToken();
nit: GetSequenceTokenForCurrentThread might be a slightly better name.  It
matches your first sentence.

[tommycli, 2013-07-08 17:42:15]

https://chromiumcodereview.appspot.com/18650006/diff/7001/base/threading/sequ...
File base/threading/sequenced_worker_pool.cc (right):

https://chromiumcodereview.appspot.com/18650006/diff/7001/base/threading/sequ...
base/threading/sequenced_worker_pool.cc:222: SequencedWorkerPool::SequenceToken>
> lazy_tls_ptr =
On 2013/07/05 05:24:19, darin wrote:
> nit: with the static member variable, you used the "g_" prefix, but with the
> file static, you did not.  was that intended?  shouldn't the file static use
> "g_" too?

Done.

https://chromiumcodereview.appspot.com/18650006/diff/7001/base/threading/sequ...
File base/threading/sequenced_worker_pool.h (right):

https://chromiumcodereview.appspot.com/18650006/diff/7001/base/threading/sequ...
base/threading/sequenced_worker_pool.h:129: // An zero |id_| is a sentinel value
internally for unsequenced tasks.
On 2013/07/05 05:24:19, darin wrote:
> nit: An -> A, and I think you also meant to put "used" after "value"
> 
> That said, this comment seems a bit odd on a public method as consumers
probably
> don't need to know about implementation details.  What do unsequenced tasks
have
> to do with a SequenceToken, which is all about sequential-izing tasks?   Who
is
> expected to call this method?
> 
> I think this is about supporting the case where the current thread is
executing
> an unsequenced task dispatched through a sequenced worker pool.  In other
words,
> there is no sequence token for such a thread.  The comment for IsValid should
> probably just state something like this.

Done.

https://chromiumcodereview.appspot.com/18650006/diff/7001/base/threading/sequ...
base/threading/sequenced_worker_pool.h:154: static SequenceToken
CurrentThreadSequenceToken();
On 2013/07/05 05:24:19, darin wrote:
> nit: GetSequenceTokenForCurrentThread might be a slightly better name.  It
> matches your first sentence.

Done.

[commit-bot: I haz the power, 2013-07-08 17:58:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18650006/21001

[commit-bot: I haz the power, 2013-07-08 18:07:24]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-08 18:19:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18650006/34001

[commit-bot: I haz the power, 2013-07-08 18:39:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-07-08 18:43:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/18650006/34001

[commit-bot: I haz the power, 2013-07-08 21:30:01]

Change committed as 210423

[Ami GONE FROM CHROMIUM, 2013-07-09 00:15:57]

https://chromiumcodereview.appspot.com/18650006/diff/34001/base/threading/seq...
File base/threading/sequenced_worker_pool.cc (right):

https://chromiumcodereview.appspot.com/18650006/diff/34001/base/threading/seq...
base/threading/sequenced_worker_pool.cc:692: TimeTicks::Now() -
shutdown_wait_begin);
This change makes shutdown_wait_begin a write-only variable on OS_NACL, which
broke this bot: 

http://build.chromium.org/p/chromium.webrtc/builders/ChromiumOS%20%5Bdaisy%5D...

Can you add ALLOW_UNUSED to the decl at l.682 or also wrap that line in a
!OS_NACL check?

[tommycli, 2013-07-09 00:23:13]

I added an !OS_NACL to that line also. Thanks!

On 2013/07/09 00:15:57, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/18650006/diff/34001/base/threading/seq...
> File base/threading/sequenced_worker_pool.cc (right):
> 
>
https://chromiumcodereview.appspot.com/18650006/diff/34001/base/threading/seq...
> base/threading/sequenced_worker_pool.cc:692: TimeTicks::Now() -
> shutdown_wait_begin);
> This change makes shutdown_wait_begin a write-only variable on OS_NACL, which
> broke this bot: 
> 
>
http://build.chromium.org/p/chromium.webrtc/builders/ChromiumOS%2520%255Bdais...
> 
> Can you add ALLOW_UNUSED to the decl at l.682 or also wrap that line in a
> !OS_NACL check?