Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165663003:
TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread (Closed)
DescriptionTaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread.
base::SequenceToken is an opaque token that identifies a series of
sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread
identifies the SequenceToken of tasks running on the current thread
throughout its lifetime.
This CL also changes SequenceChecker to support
base::internal::SequenceToken.
In an upcoming CL, base/task_scheduler will tag all its sequences with
a token returned by SequenceToken::Create(). Before running a task, it
will instantiate a ScopedSetSequenceTokenForCurrentThread with the
SequenceToken of the task's sequence.
Sample usage of SequenceToken in follow-up CL:
https://codereview.chromium.org/2172713003/
BUG=553459
Committed: https://crrev.com/eed5fa78db74a1d3d826f80baf4e6e0a188a2f43
Cr-Commit-Position: refs/heads/master@{#407937}
Patch Set 1 #
Total comments: 27
Patch Set 2 : CR gab #3 #Patch Set 3 : self-review #Patch Set 4 : self-review #Patch Set 5 : check ThreadTaskRunnerHandle #Patch Set 6 : format #Patch Set 7 : improve comment #
Total comments: 12
Patch Set 8 : CR gab #9 #
Total comments: 19
Patch Set 9 : rebase #Patch Set 10 : CR robliao #16 #Patch Set 11 : restore base\threading\thread_checker* #Patch Set 12 : invalid SequenceTokens are not equal #
Total comments: 8
Patch Set 13 : CR danakj #28 (improve comments, GetForCurrentThread, ScopedSetSequenceTokenForCurrentThread) #Patch Set 14 : self-review #
Total comments: 7
Dependent Patchsets: Messages
Total messages: 41 (12 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lg https://codereview.chromium.org/2165663003/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2165663003/diff/1/base/BUILD.gn#newcode680 base/BUILD.gn:680: "sequence_checker.h", Feels like sequence_checker.h should be alongside thread_checker.h in threading/ (and therefore sequence_token too), WDYT? I mean despite being "sequenced" it's about verifying the thread-safety behind a sequence? (let's ask dana before making a move if you agree) https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.cc File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.... base/sequence_checker_impl.cc:23: return sequence_token_.Equals(internal::SequenceToken::GetCurrent()); operator== is favored over Equals() methods in style guide (don't bother changing it for SequencedWorkerPool though) https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.h File base/sequence_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.... base/sequence_checker_impl.h:47: mutable SequencedWorkerPool::SequenceToken sequenced_worker_pool_token_; // TODO(gab): Remove this when SequencedWorkerPool is deprecated in favor of TaskScheduler. https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... File base/sequence_checker_impl_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... base/sequence_checker_impl_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. Feels this should have been detected as a copy, please adjust --similarity on upload so that this isn't a full new file. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.cc#newc... base/sequence_token.cc:37: if (!tls_current_sequence_token.Get().Get()) Store result in variable and use ternary operator for return. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:32: // SequenceToken (except in case of overflow). Remove "(except in case of overflow)" https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:36: // thread. This is set by instantiating a ScopedSetCurrentSequenceToken. s/on the thread . This is set by instantiating a ScopedSetCurrentSequenceToken./on the current thread, as determined by the active ScopedSetCurrentSequenceToken if any. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:46: // Throughout its lifetime, sets the value returned by s/sets/determines/ https://codereview.chromium.org/2165663003/diff/1/base/sequence_token_unittes... File base/sequence_token_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token_unittes... base/sequence_token_unittest.cc:38: const SequenceToken token = SequenceToken::Create(); Move outside scope (last expectation should still be false even if it's not destroyed) https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:66: // Note that in Debug mode, CalledOnValidThread() returns false when called from s/Note that in Debug mode,/Note that, when enabled,/ https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:67: // tasks posted to different SingleThreadTaskRunners, even if they happen to run s/different SingleThreadTaskRunners/SingleThreadTaskRunners bound to different sequences/ https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:68: // on the same thread. " (e.g. two independent TaskRunners with ExecutionMode::SINGLE_THREADED on the TaskScheduler that happen to share a thread)" https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... File base/threading/thread_checker_impl_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker_impl_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Same comment about finding proper --similarity on upload.
PTAnL https://codereview.chromium.org/2165663003/diff/1/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2165663003/diff/1/base/BUILD.gn#newcode680 base/BUILD.gn:680: "sequence_checker.h", On 2016/07/20 19:06:36, gab wrote: > Feels like sequence_checker.h should be alongside thread_checker.h in threading/ > (and therefore sequence_token too), WDYT? I mean despite being "sequenced" it's > about verifying the thread-safety behind a sequence? > > (let's ask dana before making a move if you agree) sgtm. However, that should be done in a separate CL. https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.cc File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.... base/sequence_checker_impl.cc:23: return sequence_token_.Equals(internal::SequenceToken::GetCurrent()); On 2016/07/20 19:06:36, gab wrote: > operator== is favored over Equals() methods in style guide (don't bother > changing it for SequencedWorkerPool though) Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.h File base/sequence_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl.... base/sequence_checker_impl.h:47: mutable SequencedWorkerPool::SequenceToken sequenced_worker_pool_token_; On 2016/07/20 19:06:36, gab wrote: > // TODO(gab): Remove this when SequencedWorkerPool is deprecated in favor of > TaskScheduler. Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... File base/sequence_checker_impl_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... base/sequence_checker_impl_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/07/20 19:06:36, gab wrote: > Feels this should have been detected as a copy, please adjust --similarity on > upload so that this isn't a full new file. This file changed too much. It isn't detected as a copy with 1% similarity. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.cc#newc... base/sequence_token.cc:37: if (!tls_current_sequence_token.Get().Get()) On 2016/07/20 19:06:36, gab wrote: > Store result in variable and use ternary operator for return. Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:32: // SequenceToken (except in case of overflow). On 2016/07/20 19:06:36, gab wrote: > Remove "(except in case of overflow)" Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:36: // thread. This is set by instantiating a ScopedSetCurrentSequenceToken. On 2016/07/20 19:06:37, gab wrote: > s/on the thread . This is set by instantiating a > ScopedSetCurrentSequenceToken./on the current thread, as determined by the > active ScopedSetCurrentSequenceToken if any. Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token.h#newco... base/sequence_token.h:46: // Throughout its lifetime, sets the value returned by On 2016/07/20 19:06:37, gab wrote: > s/sets/determines/ Done. https://codereview.chromium.org/2165663003/diff/1/base/sequence_token_unittes... File base/sequence_token_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_token_unittes... base/sequence_token_unittest.cc:38: const SequenceToken token = SequenceToken::Create(); On 2016/07/20 19:06:37, gab wrote: > Move outside scope (last expectation should still be false even if it's not > destroyed) Done. https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:66: // Note that in Debug mode, CalledOnValidThread() returns false when called from On 2016/07/20 19:06:37, gab wrote: > s/Note that in Debug mode,/Note that, when enabled,/ Done. https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:67: // tasks posted to different SingleThreadTaskRunners, even if they happen to run On 2016/07/20 19:06:37, gab wrote: > s/different SingleThreadTaskRunners/SingleThreadTaskRunners bound to different > sequences/ Done. https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker.h:68: // on the same thread. On 2016/07/20 19:06:37, gab wrote: > " (e.g. two independent TaskRunners with ExecutionMode::SINGLE_THREADED on the > TaskScheduler that happen to share a thread)" Done. https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... File base/threading/thread_checker_impl_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/threading/thread_check... base/threading/thread_checker_impl_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/07/20 19:06:37, gab wrote: > Same comment about finding proper --similarity on upload. Hmmm... git cl upload detects that this is a copy of worker_pool_unittest.cc (see previous patch set).
gab@/robliao@: PTAnL ThreadCheckerImpl::CalledOnValidThread() will currently return true if called from different SEQUENCED tasks that happen to run on the same thread. I thought about returning false when there is a SequenceToken and no ThreadTaskRunnerHandle... but this isn't correct because CalledOnValidThread() must return true if called multiple times within the same SEQUENCED task. Do you think we need a TaskToken to detect when we aren't in the same task (could be done in a separate CL)?
On 2016/07/21 15:15:42, fdoray wrote: > gab@/robliao@: PTAnL > > ThreadCheckerImpl::CalledOnValidThread() will currently return true if called > from different SEQUENCED tasks that happen to run on the same thread. I thought > about returning false when there is a SequenceToken and no > ThreadTaskRunnerHandle... but this isn't correct because CalledOnValidThread() > must return true if called multiple times within the same SEQUENCED task. Do you > think we need a TaskToken to detect when we aren't in the same task (could be > done in a separate CL)? ThreadChecker should not be used in SEQUENCED contexts, so always returning false when there is a SequenceToken but no ThreadTaskRunnerHandle. Re. returning true if called multiple times with the same SEQUENCED task -- I guess that's true but I don't think this needs to be supported. Returning false here will make it easy to catch mistakes where someone migrates a component to a SEQUENCED TaskRunner on the scheduler but forgets to change its usage of ThreadChecker to SequenceChecker.
https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... File base/sequence_checker_impl_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/1/base/sequence_checker_impl_... base/sequence_checker_impl_unittest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2016/07/21 15:02:37, fdoray wrote: > On 2016/07/20 19:06:36, gab wrote: > > Feels this should have been detected as a copy, please adjust --similarity on > > upload so that this isn't a full new file. > > This file changed too much. It isn't detected as a copy with 1% similarity. Ok, can we do the rename to _impl_unitest in a follow-up CL then so that git cl upload realizes it's the same file? (same for thread_checker_impl_unittest.cc)
PTAnL
https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... File base/sequence_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... base/sequence_checker_unittest.cc:102: internal::SequenceToken::Create()); internal::SequenceToken::Create() will become invalid after this call and the checks below are done on freed state, need to use a variable. (below as well) https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... base/sequence_checker_unittest.cc:120: Also EXPECT_TRUE(sequence_checker.CalledOnValidSequencedThread()); here to really drive the essence of this test. https://codereview.chromium.org/2165663003/diff/120001/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/120001/base/sequence_token.h#... base/sequence_token.h:12: namespace internal { I think internal:: should be reserved for implementation details referred by no others than the impl and its tests. This is intended to be used by other things (albeit very few) and as such shouldn't be internal I think. https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... base/threading/thread_checker.h:72: // posted with ExecutionMode::PARALLEL or ExecutionMode::SEQUENCED to the PARALLEL won't have a SequenceToken -- keeping the example about SEQUENCED is fine IMO https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... File base/threading/thread_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... base/threading/thread_checker_unittest.cc:105: ThreadCheckerImpl thread_checker; EXPECT_TRUE on this thread to drive essence of test.
gab@: PTAnL https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... File base/sequence_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... base/sequence_checker_unittest.cc:102: internal::SequenceToken::Create()); On 2016/07/21 21:14:24, gab wrote: > internal::SequenceToken::Create() will become invalid after this call and the > checks below are done on freed state, need to use a variable. > > (below as well) ScopedSetCurrentSequenceToken copies the argument it receives, so this shouldn't be an issue. https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... base/sequence_checker_unittest.cc:120: On 2016/07/21 21:14:24, gab wrote: > Also EXPECT_TRUE(sequence_checker.CalledOnValidSequencedThread()); here to > really drive the essence of this test. Done. https://codereview.chromium.org/2165663003/diff/120001/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/120001/base/sequence_token.h#... base/sequence_token.h:12: namespace internal { On 2016/07/21 21:14:24, gab wrote: > I think internal:: should be reserved for implementation details referred by no > others than the impl and its tests. > > This is intended to be used by other things (albeit very few) and as such > shouldn't be internal I think. Done. https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... base/threading/thread_checker.h:72: // posted with ExecutionMode::PARALLEL or ExecutionMode::SEQUENCED to the On 2016/07/21 21:14:24, gab wrote: > PARALLEL won't have a SequenceToken -- keeping the example about SEQUENCED is > fine IMO keeping the example about SEQUENCED to avoid confusion sgtm. But PARALLEL tasks are in a single-task sequence and hence have a SequenceToken. https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... File base/threading/thread_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... base/threading/thread_checker_unittest.cc:105: ThreadCheckerImpl thread_checker; On 2016/07/21 21:14:24, gab wrote: > EXPECT_TRUE on this thread to drive essence of test. Done.
lgtm, refer to your follow-up CL in this CL's description as an example user, thanks! https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... File base/sequence_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/120001/base/sequence_checker_... base/sequence_checker_unittest.cc:102: internal::SequenceToken::Create()); On 2016/07/25 13:24:28, fdoray wrote: > On 2016/07/21 21:14:24, gab wrote: > > internal::SequenceToken::Create() will become invalid after this call and the > > checks below are done on freed state, need to use a variable. > > > > (below as well) > > ScopedSetCurrentSequenceToken copies the argument it receives, so this shouldn't > be an issue. Ah, good point :-). https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... File base/threading/thread_checker.h (right): https://codereview.chromium.org/2165663003/diff/120001/base/threading/thread_... base/threading/thread_checker.h:72: // posted with ExecutionMode::PARALLEL or ExecutionMode::SEQUENCED to the On 2016/07/25 13:24:28, fdoray wrote: > On 2016/07/21 21:14:24, gab wrote: > > PARALLEL won't have a SequenceToken -- keeping the example about SEQUENCED is > > fine IMO > > keeping the example about SEQUENCED to avoid confusion sgtm. But PARALLEL tasks > are in a single-task sequence and hence have a SequenceToken. Ah right, but IMO that's an implementation detail of the scheduler (because the queued type internally happens to be sequences). Don't think we should make API guarantees about that though.
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker and ThreadChecker to take advantage of base::internal::SequenceToken. When the token returned by SequenceToken::GetCurrent() is valid, SequenceChecker and ThreadChecker require that it remains the same across calls to CalledOnValid(Sequenced)Thread(). Note that ThreadChecker::CalledOnValidThread() will complain if called on the same thread with different sequence tokens. This is meant to detect tasks posted to different single-threaded TaskRunners that happen to run on the same thread. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker and ThreadChecker to take advantage of base::internal::SequenceToken. When the token returned by SequenceToken::GetCurrent() is valid, SequenceChecker and ThreadChecker require that it remains the same across calls to CalledOnValid(Sequenced)Thread(). Note that ThreadChecker::CalledOnValidThread() will complain if called on the same thread with different sequence tokens. This is meant to detect tasks posted to different single-threaded TaskRunners that happen to run on the same thread. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
robliao@: PTAL
fdoray@chromium.org changed reviewers: + danakj@chromium.org
+danakj@: PTAL
It seems that the thread checker changes can plausibly be done as a separate CL. Would that be appropriate? https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.cc:52: DCHECK(!sequence_token_.IsValid() || !sequenced_worker_pool_token_.IsValid()); And a quick comment for this DCHECK as to why we don't expect both sequence_token and sequenced_worker_pool_token_ to be set (e.g. the TaskScheduler doesn't use sequenced_worker_pool_token_) https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.h:18: // Real implementation of SequenceChecker, for use in debug mode, or for Nit: Remove commas on this line. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.h:23: // version for your build configuration. Are there legitimate uses for direct access? Maybe this should go in base::internal. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_unittest.cc:185: PostToSequencedWorkerPool( Are 4 identical calls necessary to perform this test? Can we use 2? https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; Are invalid tokens equal to other invalid tokens? https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:28: return !(*this == other); It seems strange that we're doing a pointer comparison here and a token comparison above. This should probably do the opposite as ==. https://codereview.chromium.org/2165663003/diff/140001/base/threading/thread_... File base/threading/thread_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/140001/base/threading/thread_... base/threading/thread_checker_impl.h:40: mutable base::Lock lock_; Observation: Since this entire class is mutable, I'm not sure how much value those const's provide.
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker and ThreadChecker to take advantage of base::internal::SequenceToken. When the token returned by SequenceToken::GetCurrent() is valid, SequenceChecker and ThreadChecker require that it remains the same across calls to CalledOnValid(Sequenced)Thread(). Note that ThreadChecker::CalledOnValidThread() will complain if called on the same thread with different sequence tokens. This is meant to detect tasks posted to different single-threaded TaskRunners that happen to run on the same thread. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to use base::internal::SequenceToken. When the token returned by SequenceToken::GetCurrent() is valid, SequenceChecker requires that it remains the same across calls to CalledOnValidThread(). Note that ThreadChecker::CalledOnValidThread() will complain if called on the same thread with different sequence tokens. This is meant to detect tasks posted to different single-threaded TaskRunners that happen to run on the same thread. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to use base::internal::SequenceToken. When the token returned by SequenceToken::GetCurrent() is valid, SequenceChecker requires that it remains the same across calls to CalledOnValidThread(). Note that ThreadChecker::CalledOnValidThread() will complain if called on the same thread with different sequence tokens. This is meant to detect tasks posted to different single-threaded TaskRunners that happen to run on the same thread. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
robliao@: PTAnL https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.cc:52: DCHECK(!sequence_token_.IsValid() || !sequenced_worker_pool_token_.IsValid()); On 2016/07/25 18:20:44, robliao wrote: > And a quick comment for this DCHECK as to why we don't expect both > sequence_token and sequenced_worker_pool_token_ to be set (e.g. the > TaskScheduler doesn't use sequenced_worker_pool_token_) Done. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.h:18: // Real implementation of SequenceChecker, for use in debug mode, or for On 2016/07/25 18:20:44, robliao wrote: > Nit: Remove commas on this line. Done. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_impl.h:23: // version for your build configuration. On 2016/07/25 18:20:44, robliao wrote: > Are there legitimate uses for direct access? Maybe this should go in > base::internal. ThreadCheckerImpl in unit tests outside of base/: https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide... (to verify that a task runs on the appropriate thread even when !DCHECK_IS_ON()) SequenceCheckerImpl could be used in a similar way. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... File base/sequence_checker_unittest.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_checker_... base/sequence_checker_unittest.cc:185: PostToSequencedWorkerPool( On 2016/07/25 18:20:44, robliao wrote: > Are 4 identical calls necessary to perform this test? Can we use 2? Done. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; On 2016/07/25 18:20:44, robliao wrote: > Are invalid tokens equal to other invalid tokens? Yes, as tested by SequenceTokenTest.OperatorEquals. https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:28: return !(*this == other); On 2016/07/25 18:20:44, robliao wrote: > It seems strange that we're doing a pointer comparison here and a token > comparison above. This should probably do the opposite as ==. This isn't a pointer comparison. I just implemented operator!= in terms of operator==. https://codereview.chromium.org/2165663003/diff/140001/base/threading/thread_... File base/threading/thread_checker_impl.h (right): https://codereview.chromium.org/2165663003/diff/140001/base/threading/thread_... base/threading/thread_checker_impl.h:40: mutable base::Lock lock_; On 2016/07/25 18:20:44, robliao wrote: > Observation: Since this entire class is mutable, I'm not sure how much value > those const's provide. The fact that CalledOnValidThread() is const allows it to be called from const methods. E.g.: class MyClass { public: void MyConstMethod() const { DCHECK(thread_checker_.CalledOnValidThread()); ... } private: ThreadChecker thread_checker_; };
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::internal::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
Ping on. > It seems that the thread checker changes can plausibly be done as a separate CL. >Would that be appropriate? https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; On 2016/07/25 19:44:29, fdoray wrote: > On 2016/07/25 18:20:44, robliao wrote: > > Are invalid tokens equal to other invalid tokens? > > Yes, as tested by SequenceTokenTest.OperatorEquals. I should have phrased this, should they be equal to other invalid tokens? The proposed change would be token_ == other.token_ && token_ != kInvalidSequenceToken https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:28: return !(*this == other); On 2016/07/25 19:44:29, fdoray wrote: > On 2016/07/25 18:20:44, robliao wrote: > > It seems strange that we're doing a pointer comparison here and a token > > comparison above. This should probably do the opposite as ==. > > This isn't a pointer comparison. I just implemented operator!= in terms of > operator==. Ah, yes. I misread this code!
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
On 2016/07/25 19:49:31, robliao wrote: > Ping on. > > > It seems that the thread checker changes can plausibly be done as a separate > CL. > >Would that be appropriate? > > https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc > File base/sequence_token.cc (right): > > https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... > base/sequence_token.cc:24: return token_ == other.token_; > On 2016/07/25 19:44:29, fdoray wrote: > > On 2016/07/25 18:20:44, robliao wrote: > > > Are invalid tokens equal to other invalid tokens? > > > > Yes, as tested by SequenceTokenTest.OperatorEquals. > > I should have phrased this, should they be equal to other invalid tokens? > > The proposed change would be > token_ == other.token_ && token_ != kInvalidSequenceToken > > https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... > base/sequence_token.cc:28: return !(*this == other); > On 2016/07/25 19:44:29, fdoray wrote: > > On 2016/07/25 18:20:44, robliao wrote: > > > It seems strange that we're doing a pointer comparison here and a token > > > comparison above. This should probably do the opposite as ==. > > > > This isn't a pointer comparison. I just implemented operator!= in terms of > > operator==. > > Ah, yes. I misread this code! Gotcha. Addressed in the latest patchset.
lgtm
danakj@: PTAL https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; On 2016/07/25 19:49:31, robliao wrote: > On 2016/07/25 19:44:29, fdoray wrote: > > On 2016/07/25 18:20:44, robliao wrote: > > > Are invalid tokens equal to other invalid tokens? > > > > Yes, as tested by SequenceTokenTest.OperatorEquals. > > I should have phrased this, should they be equal to other invalid tokens? > > The proposed change would be > token_ == other.token_ && token_ != kInvalidSequenceToken I don't see a compelling reason why two invalid SequenceTokens wouldn't be equal. To me, this isn't "unsurprising" https://google.github.io/styleguide/cppguide.html#Operator_Overloading: SequenceToken invalid_a; SequenceToken invalid_b = invalid_a; invalid_a == invalid_b; // false!
https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; On 2016/07/25 20:03:21, fdoray wrote: > On 2016/07/25 19:49:31, robliao wrote: > > On 2016/07/25 19:44:29, fdoray wrote: > > > On 2016/07/25 18:20:44, robliao wrote: > > > > Are invalid tokens equal to other invalid tokens? > > > > > > Yes, as tested by SequenceTokenTest.OperatorEquals. > > > > I should have phrased this, should they be equal to other invalid tokens? > > > > The proposed change would be > > token_ == other.token_ && token_ != kInvalidSequenceToken > > I don't see a compelling reason why two invalid SequenceTokens wouldn't be > equal. To me, this isn't "unsurprising" > https://google.github.io/styleguide/cppguide.html#Operator_Overloading: > > SequenceToken invalid_a; > SequenceToken invalid_b = invalid_a; > invalid_a == invalid_b; // false! It would be closing the door on folks attempting to check invalidity through ==. It's not too different from floats returning false for all NaN's passed through ==. http://stackoverflow.com/questions/1565164/what-is-the-rationale-for-all-comp...
danakj@: PTAL https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc File base/sequence_token.cc (right): https://codereview.chromium.org/2165663003/diff/140001/base/sequence_token.cc... base/sequence_token.cc:24: return token_ == other.token_; On 2016/07/25 20:10:47, robliao wrote: > On 2016/07/25 20:03:21, fdoray wrote: > > On 2016/07/25 19:49:31, robliao wrote: > > > On 2016/07/25 19:44:29, fdoray wrote: > > > > On 2016/07/25 18:20:44, robliao wrote: > > > > > Are invalid tokens equal to other invalid tokens? > > > > > > > > Yes, as tested by SequenceTokenTest.OperatorEquals. > > > > > > I should have phrased this, should they be equal to other invalid tokens? > > > > > > The proposed change would be > > > token_ == other.token_ && token_ != kInvalidSequenceToken > > > > I don't see a compelling reason why two invalid SequenceTokens wouldn't be > > equal. To me, this isn't "unsurprising" > > https://google.github.io/styleguide/cppguide.html#Operator_Overloading: > > > > SequenceToken invalid_a; > > SequenceToken invalid_b = invalid_a; > > invalid_a == invalid_b; // false! > > It would be closing the door on folks attempting to check invalidity through ==. > > It's not too different from floats returning false for all NaN's passed through > ==. > > http://stackoverflow.com/questions/1565164/what-is-the-rationale-for-all-comp... Done.
LGTM w/ nits https://codereview.chromium.org/2165663003/diff/220001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/220001/base/sequence_checker.... base/sequence_checker.h:34: // posted to SequencedTaskRunners bound to the same sequence or from a single or *to* a single thread, right? https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:24: bool operator==(const SequenceToken& other) const; Can you document that an invalid token is not equal to any other token, including other invalid tokens. https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:36: static SequenceToken GetCurrent(); nit: I'ld like GetForCurrentThread() a bit better to point out this is using TLS. https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:47: class BASE_EXPORT ScopedSetCurrentSequenceToken { and ScopedSetSequenceTokenForCurrentThread?
https://codereview.chromium.org/2165663003/diff/220001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/220001/base/sequence_checker.... base/sequence_checker.h:34: // posted to SequencedTaskRunners bound to the same sequence or from a single On 2016/07/26 19:34:37, danakj wrote: > or *to* a single thread, right? No. I rephrased this comment. CalledOnValidSequencedThread() will return false if called from SingleThreadTaskRunners bound to different sequences, even if they happen to run on the same thread. https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h File base/sequence_token.h (right): https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:24: bool operator==(const SequenceToken& other) const; On 2016/07/26 19:34:37, danakj wrote: > Can you document that an invalid token is not equal to any other token, > including other invalid tokens. Done. https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:36: static SequenceToken GetCurrent(); On 2016/07/26 19:34:37, danakj wrote: > nit: I'ld like GetForCurrentThread() a bit better to point out this is using > TLS. Done. https://codereview.chromium.org/2165663003/diff/220001/base/sequence_token.h#... base/sequence_token.h:47: class BASE_EXPORT ScopedSetCurrentSequenceToken { On 2016/07/26 19:34:37, danakj wrote: > and ScopedSetSequenceTokenForCurrentThread? Done.
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetCurrentSequenceToken. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetCurrentSequenceToken identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetCurrentSequenceToken with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetSequenceTokenForCurrentThread with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2165663003/#ps260001 (title: "self-review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still lgtm but one thought on latest changes https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.... base/sequence_checker.h:36: // - From a single thread outside of any task. I find this last statement confusing, MessageLoop as SingleThreadTaskRunners but they don't have a "sequence" per se (i.e. no SequenceToken), which category are they in? https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker_... File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker_... base/sequence_checker_impl.cc:22: if (sequence_token_.IsValid()) Do we even need IsValid() in a world where invalid tokens aren't equal? I guess it might be more readable but if (sequence_token_ == SequenceToken::GetForCurrentThread()) return true; would have the same logic :-)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetSequenceTokenForCurrentThread with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetSequenceTokenForCurrentThread with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetSequenceTokenForCurrentThread with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 ========== to ========== TaskScheduler: Add SequenceToken and ScopedSetSequenceTokenForCurrentThread. base::SequenceToken is an opaque token that identifies a series of sequenced tasks. base::internal::ScopedSetSequenceTokenForCurrentThread identifies the SequenceToken of tasks running on the current thread throughout its lifetime. This CL also changes SequenceChecker to support base::internal::SequenceToken. In an upcoming CL, base/task_scheduler will tag all its sequences with a token returned by SequenceToken::Create(). Before running a task, it will instantiate a ScopedSetSequenceTokenForCurrentThread with the SequenceToken of the task's sequence. Sample usage of SequenceToken in follow-up CL: https://codereview.chromium.org/2172713003/ BUG=553459 Committed: https://crrev.com/eed5fa78db74a1d3d826f80baf4e6e0a188a2f43 Cr-Commit-Position: refs/heads/master@{#407937} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/eed5fa78db74a1d3d826f80baf4e6e0a188a2f43 Cr-Commit-Position: refs/heads/master@{#407937}
Message was sent while issue was closed.
https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.... base/sequence_checker.h:25: bool CalledOnValidSequencedThread() const { Just had a post-commit thought when looking at other code: CalledOnValidSequencedThread() is the wrong name for this method after this change, CalledOnValidSequence() would be more appropriate (the old name works when you verify you're on any thread in a pool, but now that we closely verify tokens it goes beyond that -- I guess it did verify SWP tokens before but eh this CL increases this gap). Plus CalledOnValidSequence() is a shorter and nicer name anyways :-). WDYT?
Message was sent while issue was closed.
https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.... base/sequence_checker.h:25: bool CalledOnValidSequencedThread() const { On 2016/07/28 15:16:41, gab wrote: > Just had a post-commit thought when looking at other code: > > CalledOnValidSequencedThread() is the wrong name for this method after this > change, CalledOnValidSequence() would be more appropriate (the old name works > when you verify you're on any thread in a pool, but now that we closely verify > tokens it goes beyond that -- I guess it did verify SWP tokens before but eh > this CL increases this gap). > > Plus CalledOnValidSequence() is a shorter and nicer name anyways :-). > > WDYT? I agree! Working on a CL that makes this change. https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.... base/sequence_checker.h:36: // - From a single thread outside of any task. On 2016/07/26 20:49:48, gab wrote: > I find this last statement confusing, MessageLoop as SingleThreadTaskRunners but > they don't have a "sequence" per se (i.e. no SequenceToken), which category are > they in? I think we don't plan to support child single-threaded TaskRunners in base/task_scheduler? If that's true, then this comment could become: [...] Calls are sequenced when they are issued: * From tasks posted to SequencedTaskRunners bound to the same sequence. (task_scheduler and SequencedWorkerPool can have different SequencedTaskRunners bound to the same sequence) * From tasks posted to the same SingleThreadTaskRunner. * From a single thread, outside of any task. https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker_... File base/sequence_checker_impl.cc (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker_... base/sequence_checker_impl.cc:22: if (sequence_token_.IsValid()) On 2016/07/26 20:49:49, gab wrote: > Do we even need IsValid() in a world where invalid tokens aren't equal? I guess > it might be more readable but > > if (sequence_token_ == SequenceToken::GetForCurrentThread()) > return true; > > would have the same logic :-) If we do that, we'll delegate to |thread_checker_| when |sequence_token_| is valid and SequenceToken::GetForCurrentThread() isn't, which isn't the desired behavior. ThreadChecker also needs IsValid(). When ThreadChecker::sequence_token_ != SequenceToken::GetForCurrentThread(), we need to know if ThreadChecker::sequence_token_ is valid to decide whether we return false right away or check the thread id.
Message was sent while issue was closed.
https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.h File base/sequence_checker.h (right): https://codereview.chromium.org/2165663003/diff/260001/base/sequence_checker.... base/sequence_checker.h:36: // - From a single thread outside of any task. On 2016/07/28 18:10:04, fdoray wrote: > On 2016/07/26 20:49:48, gab wrote: > > I find this last statement confusing, MessageLoop as SingleThreadTaskRunners > but > > they don't have a "sequence" per se (i.e. no SequenceToken), which category > are > > they in? > > I think we don't plan to support child single-threaded TaskRunners in > base/task_scheduler? If that's true, then this comment could become: > > [...] Calls are sequenced when they are issued: > * From tasks posted to SequencedTaskRunners bound to the same sequence. > (task_scheduler and SequencedWorkerPool can have different SequencedTaskRunners bound to the same sequence) I don't think this (..) precision is required. > * From tasks posted to the same SingleThreadTaskRunner. > * From a single thread, outside of any task. Otherwise yes sgtm (and we can adjust the comment if we decide to allow SingleThreadTaskRunners to share a sequence later). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
