Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1704113002:
TaskScheduler [6] SchedulerWorkerThread (Closed)
DescriptionTaskScheduler [6] SchedulerWorkerThread
A SchedulerWorkerThread runs Tasks from Sequences returned by a custom
callback.
This change is a subset of https://codereview.chromium.org/1698183005/
BUG=553459
CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org
Committed: https://crrev.com/93649e8fc3faf88cee5a9b03b67199c4bb614b1c
Cr-Commit-Position: refs/heads/master@{#384441}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase #
Total comments: 4
Patch Set 5 : CR from robliao #
Total comments: 93
Patch Set 6 : rebase #Patch Set 7 : CR from gab #18 #
Total comments: 54
Patch Set 8 : CR robliao #22 #Patch Set 9 : rebase #Patch Set 10 : self-review #Patch Set 11 : Experiment: State changes within the scope of a shared PQ Transaction. #Patch Set 12 : self review #
Total comments: 9
Patch Set 13 : Experiment: Get work from a callback. #Patch Set 14 : self review #
Total comments: 13
Patch Set 15 : CR robliao #31 #
Total comments: 22
Patch Set 16 : CR danakj #36 #Patch Set 17 : format #Patch Set 18 : add main entry callback #
Total comments: 27
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 53 (16 generated)
Description was changed from ========== TaskScheduler [6/8] WorkerThread R=gab@chromium.org, robliao@chromium.org ========== to ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ==========
Description was changed from ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ As documented in https://docs.google.com/a/chromium.org/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrk... BUG=553459 ========== to ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ==========
Description was changed from ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 ========== to ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=vmpstr+watch@chromium.org ==========
Description was changed from ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... base/task_scheduler/worker_thread.cc:26: class SchedulerSingleThreadTaskRunner : public SingleThreadTaskRunner { Working through the ChildTaskRunner, do these TaskRunners have to extend the specific type (SingleThreadTaskRunner, SequencedTaskRunner)? We only return TaskRunners, and things appear to work fine if they all extend TaskRunner instead.
https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... base/task_scheduler/worker_thread.cc:26: class SchedulerSingleThreadTaskRunner : public SingleThreadTaskRunner { On 2016/03/07 19:28:08, robliao wrote: > Working through the ChildTaskRunner, do these TaskRunners have to extend the > specific type (SingleThreadTaskRunner, SequencedTaskRunner)? We only return > TaskRunners, and things appear to work fine if they all extend TaskRunner > instead. They do, we will eventually need the underlying types to set ThreadTaskRunnerHandles (ref. a_3_thread_handles in private repo).
On 2016/03/07 19:28:08, robliao wrote: > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > File base/task_scheduler/worker_thread.cc (right): > > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > base/task_scheduler/worker_thread.cc:26: class SchedulerSingleThreadTaskRunner : > public SingleThreadTaskRunner { > Working through the ChildTaskRunner, do these TaskRunners have to extend the > specific type (SingleThreadTaskRunner, SequencedTaskRunner)? We only return > TaskRunners, and things appear to work fine if they all extend TaskRunner > instead. The TaskRunners that we return don't have to extend specific task runners types. However, we'll need special task runners that extend SingleThreadTaskRunner/SequencedTaskRunner and wrap a scheduler task runner to support ThreadTaskRunner/SequencedTaskRunner. Something like this: class SchedulerSingleThreadTaskRunner : public SingleThreadTaskRunner { public: bool PostTask(...) override { return single_thread_task_runner_->PostTask(...); } private: scoped_refptr<SchedulerTaskRunner> single_thread_task_runner_; };
On 2016/03/07 19:33:40, fdoray wrote: > On 2016/03/07 19:28:08, robliao wrote: > > > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > > File base/task_scheduler/worker_thread.cc (right): > > > > > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > > base/task_scheduler/worker_thread.cc:26: class SchedulerSingleThreadTaskRunner > : > > public SingleThreadTaskRunner { > > Working through the ChildTaskRunner, do these TaskRunners have to extend the > > specific type (SingleThreadTaskRunner, SequencedTaskRunner)? We only return > > TaskRunners, and things appear to work fine if they all extend TaskRunner > > instead. > > The TaskRunners that we return don't have to extend specific task runners types. > However, we'll need special task runners that extend > SingleThreadTaskRunner/SequencedTaskRunner and wrap a scheduler task runner to > support ThreadTaskRunner/SequencedTaskRunner. Something like this: > > class SchedulerSingleThreadTaskRunner : public SingleThreadTaskRunner { > public: > bool PostTask(...) override { > return single_thread_task_runner_->PostTask(...); > } > private: > scoped_refptr<SchedulerTaskRunner> single_thread_task_runner_; > }; Yep. If we actually needed the underlying functionality (and it looks like we do), the next thing to do is to simple have a SchedulerTaskRunner wrapper for the specific TaskRunner.
On 2016/03/07 19:36:30, robliao wrote: > On 2016/03/07 19:33:40, fdoray wrote: > > On 2016/03/07 19:28:08, robliao wrote: > > > > > > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > > > File base/task_scheduler/worker_thread.cc (right): > > > > > > > > > https://codereview.chromium.org/1704113002/diff/1/base/task_scheduler/worker_... > > > base/task_scheduler/worker_thread.cc:26: class > SchedulerSingleThreadTaskRunner > > : > > > public SingleThreadTaskRunner { > > > Working through the ChildTaskRunner, do these TaskRunners have to extend the > > > specific type (SingleThreadTaskRunner, SequencedTaskRunner)? We only return > > > TaskRunners, and things appear to work fine if they all extend TaskRunner > > > instead. > > > > The TaskRunners that we return don't have to extend specific task runners > types. > > However, we'll need special task runners that extend > > SingleThreadTaskRunner/SequencedTaskRunner and wrap a scheduler task runner to > > support ThreadTaskRunner/SequencedTaskRunner. Something like this: > > > > class SchedulerSingleThreadTaskRunner : public SingleThreadTaskRunner { > > public: > > bool PostTask(...) override { > > return single_thread_task_runner_->PostTask(...); > > } > > private: > > scoped_refptr<SchedulerTaskRunner> single_thread_task_runner_; > > }; > > Yep. If we actually needed the underlying functionality (and it looks like we > do), the next thing to do is to simple have a SchedulerTaskRunner wrapper for > the specific TaskRunner. An interesting consideration is what if someone creates a child task runner from the task runner from a handle? We're back at square one again!
> An interesting consideration is what if someone creates a child task runner from > the task runner from a handle? We're back at square one again! I would say that this isn't supported. To create a child task runner, a reference to a SchedulerTaskRunner is required.
I rebased this CL on the latest TaskTracker CL. Can you review it? Thanks.
https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:210: bool sequence_is_single_threaded = false; For unit tests: If we're exiting the test, call WakeUp, and the thread has not marked itself as is_awake_ = false, WakeUp will have no effect. if is_awake_ is false and we call WakeUp, we will signal the condition variable. If no thread is waiting, the signal will have no effect. Fixed by rechecking should_exit_for_testing in: https://luckyluke-private.googlesource.com/src/+/c8f5bbaff4f03637366ba67b9ab4... We'll probably need a closer audit of the state in ThreadMain. https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:27: worker_thread_(WorkerThread::CreateWorkerThread( The worker thread may start and call the idle callback before the idle callback CV has been initialized due to the order. Fixed in https://luckyluke-private.googlesource.com/src/+/0041eafd8efbdac3aa5ebbdf1e01...
Description was changed from ========== TaskScheduler [6/8] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6/9] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:210: bool sequence_is_single_threaded = false; On 2016/03/09 15:41:12, robliao wrote: > For unit tests: > If we're exiting the test, call WakeUp, and the thread has not marked itself > as is_awake_ = false, WakeUp will have no effect. > > if is_awake_ is false and we call WakeUp, we will signal the condition > variable. If no thread is waiting, the signal will have no effect. > > Fixed by rechecking should_exit_for_testing in: > https://luckyluke-private.googlesource.com/src/+/c8f5bbaff4f03637366ba67b9ab4... > > We'll probably need a closer audit of the state in ThreadMain. You're right. Thanks for the fix. I moved it inside ThreadMain, where it was easier to explain it. https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/60001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:27: worker_thread_(WorkerThread::CreateWorkerThread( On 2016/03/09 15:41:12, robliao wrote: > The worker thread may start and call the idle callback before the idle callback > CV has been initialized due to the order. > > Fixed in > https://luckyluke-private.googlesource.com/src/+/0041eafd8efbdac3aa5ebbdf1e01... Thanks!
Phew! Sorry for the delay, this is quite an advanced component, looks great overall! Mostly nits and a few fundamental comments but nothing that changes the whole design I think :-) Thanks! Gab https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... File base/task_scheduler/priority_queue.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... base/task_scheduler/priority_queue.cc:19: : sequence(sequence), sort_key(sort_key) {} std::move(sequence) (I know it already landed but just spotted this one while looking for more instances of other missing moves I find in this file) Not sure if compiler can figure out by itself that this can be moved as it's not referenced after..? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:22: SequenceSortKey sequence_sort_key = sequence->GetSortKey(); const https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:24: new PriorityQueue::SequenceAndSortKey(sequence, sequence_sort_key))); std::move(sequence) Not sure whether compiler can figure out that this is movable as it's not referenced after but moving it makes the intent explicit. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:34: DCHECK(task.get()); No .get() for bool operations here and below https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:39: task_tracker->PostTask(Bind(&PostTaskCallback, sequence, priority_queue), std::move(sequence) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.h:20: // If TaskTracker allows |task| to be posted, inserts it into |sequence|. Then, s/TaskTracker/|task_tracker|/ https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. TODO(gab): Review this file. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:31: // Post a task. s/a/the/ https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:37: // Expect to find the task in the sequence. // Expect to find the task alone in the sequence. ^^ (i.e. to highlight why you're testing a PopTask() after) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:63: // Expect to find the task in the sequence. // Expect to find the task in the sequence behind the initial task. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:64: sequence->PopTask(); EXPECT_NE(task_raw, sequence->PeekTask()); before first pop. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:69: return true; I think this can be implemented relatively easily in this by: 1) Having a scoped_ptr<ThreadCheckerImpl> member in WorkerThread which is initialized on from ThreadMain(). 2) Having this SchedulerSingleThreadTaskRunner task a non-null ThreadCheckerImpl* in constructor. 3) Verifying against it here. This is akin to how MessageLoopTaskRunner implements it [1], but I prefer using ThreadCheckerImpl as it essentially does the same thing while hiding the low-level details in a single member. Note: I'd add this to (1): // A ThreadChecker to be used to verify that the current thread is this // WorkerThread. Uses ThreadCheckerImpl instead of ThreadChecker to be able to // support the non-dcheck only RunsTasksOnCurrentThread() SingleThreadTaskRunner // API. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:76: return PostDelayedTask(from_here, task, delay); Add: // Tasks are never nested on WorkerThread. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:115: wake_up_cv_->Signal(); tl;dr; no explicit request here, mostly thoughts and looking for your opinion. I find this kind of hard to read without assumptions as nothing tells me |wake_up_cv_| is a CV on |lock_| without double-checking the constructor. Not sure what to do about this though... comment below about using it as the bool solves one side of the issue on the Wait() side where it is created, but the Signal() side still comes out of the blue. I guess this is always a problem with CVs unless there'd be a wrapper to hold both a lock and its corresponding CV. But if Chrome hasn't needed this yet perhaps it's also okay for us to have them side-by-side. EDIT: Reading the test code now which uses multiple CVs for the same lock I see how it makes sense to decouple the two. I guess it's implicit that the CV is based on the only Lock in this class and then its name indicates what it's used for. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:123: traits, &single_thread_priority_queue_, task_tracker_)); This is only fine because we never delete WorkerThreads outside tests (in which no tasks are ever posted after they're deleted). Add a comment explaining why leaking raw pointers to members is okay. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:158: // Mac only supports 2 priorities. crbug.com/554651 Looks like this is fixed, I think we can remove the Mac only code here. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:166: PlatformThread::CreateWithPriority(kDefaultStackSize, this, &thread_handle_, In theory we'd need to use CreateNonJoinable() -- or introduce CreateNonJoinableWithPriority() -- since we don't join our threads on shutdown (right?). We do wait for work queues to be empty though. Looks like we need to understand the impact of a non-joinable thread. AFAICT it only prevents use of singletons, which is probably too strict for the way we handle things. That restrictions is probably merely so that a singleton isn't in use while it gets destroyed (late on main thread by AtExitManager or statically I think). In which case we could be fine enforcing ourselves that singletons not be allowed in the scope of CONTINUE_ON_SHUTDOWN tasks, but be okay on others. Today, the POSIX WorkerPool uses this, but SequencedWorkerPool doesn't -- which is probably a bug (or at least a failure to check for potential bugs w.r.t. CONTINUE_ON_SHUTDOWN). https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:189: (!shared_sequence.is_null() && This check is redundant. If single_thread_sequence.is_null() then it's impossible that shared_sequence.is_null() or the condition on line 184 would have been true. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:204: wake_up_cv_->Wait(); So long as WaitUntilWakeUp() is only called from one thread, i.e. single waiter (can enforce with the ThreadCheckerImpl proposed above). Then I think we can do the same trick we did in TaskTracker and use the CV's existence as the bool. I guess it depends how often we think we'll need to re-create this CV and whether that would result in heap churn (the Shutdown case in TaskTracker was an easy case, this one is less clear I just tend to not like things that have more or less the same state across two members). https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:208: while (!should_exit_for_testing_) { Add a comment explaining what happens in product (i.e. run out of task, sleep, and get killed by the OS on main exit) and why this is considered preferable to joining all threads. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:212: if (sequence.get() == nullptr) { if (!sequence) (here and below) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:221: // |shared_priority_queue_| after the first call to GetWork() but before Could also have been added to |single_thread_priority_queue|, no? The way I understand this, this is: // Check one more time if there is work available. Work could have been added // between the first call to GetWork() and the |becomes_idle_callback_| // invocation in which case this WorkerThread shouldn't go to sleep. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:224: sequence = GetWork(&sequence_is_single_threaded); If we do get work here don't we need to self-wake, i.e. mark |is_awake_ = true;|? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:230: if (should_exit_for_testing_) Need to lock to check this member, right? Can we avoid having an AutoLock in product code for test-only code though? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:235: sequence = GetWork(&sequence_is_single_threaded); Here I'd say "continue;" is easier to read, results in the same behavior but makes one less piece in logic, i.e. "restart" instead of "re-get work and hope to get something but we still might not". Also "continue;" allows us to get rid of the null check on line 239 :-) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:247: PushSequenceInPriorityQueue(std::move(sequence), Inline this method here? It's a 2 lines helper and only used here. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:30: // Callback invoked by |worker_thread| to reinsert |sequence| in the The callback contract can't really dictate who calls it. So maybe instead: // Callback invoked to reinsert |sequence| in the appropriate PriorityQueue after one of its tasks was executed on |worker_thread|. And actually this comment should probably not mention "priority queue" either as nothing forces the destination to be a PQ. Two options: 1) Make the comment more generic. 2) Define the type on PriorityQueue instead of here. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:31: // appropriate priority queue it has executed one of its tasks. missing "after" before "it"? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:40: // a single-thread priority queue and from |shared_priority_queue|. s/a single-thread priority queue/its single-thread priority queue/ https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:51: // A WorkerThread must only be destroyed after JoinForTesting() has returned. Hmmm, isn't the WorkerThread destroyed in the product where JoinForTesting() is an illegal call? Or I guess we decided to never delete scheduler objects on product shutdown? If that's the reason, this should be highlighted in this comment. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:60: // scheduling tasks on this WorkerThread with |traits|. s/tasks on this WorkerThread with |traits|/tasks with |traits| on this WorkerThread/ (i.e. makes it clearer that |traits| apply to tasks not WorkerThread) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:79: scoped_refptr<Sequence> GetWork(bool* single_thread); s/single_thread/is_single_thread/ https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:82: void WaitUntilWakeUp(); WaitUntilWokenUp or WaitForWakeUp? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:97: // True from the time WakeUp() is called to the time the WorkerThread doesn't s/the WorkerThread/this WorkerThread/ (same below) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:55: void WaitUntilLastPostedTaskHasRun() { s/HasRun/Ran/ (here and below) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:104: ran_tasks_in_wrong_order_ = true; ADD_FAILURE here too instead of maintaining extra state (see below) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:110: void RunTaskThatShouldNotRun() { ran_task_that_should_not_run_ = true; } Instead of having extra state here and checking for it, this method's body could simply be: { ADD_FAILURE() << "Unexpected task execution."; } which is equivalent to EXPECT_FALSE(ran_task_that_should_not_run()); after + makes error report synchronous instead of after the fact :-) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:112: // Lock protecting |run_task_cv_|. Hmm, it does more than that it seems? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:139: ASSERT_NE(nullptr, worker_thread_.get()); ASSERT_TRUE(worker_thread_); https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:140: WaitUntilNumBecomesIdleCallbackInvocations(1); Move these two lines to constructor instead of at beginning of each test? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:145: WaitUntilNumBecomesIdleCallbackInvocations(2); Since WaitUntilNumBecomesIdleCallbackInvocations(1); will move to constructor, it will be less obvious why this is "2". So maybe instead it should just be WaitForNextBecomesIdleCallbackInvocation(); ? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:147: worker_thread_->JoinForTesting(); Since the thread is created by the fixture I think JoinForTesting() belongs in the fixture's TearDown(). (and maybe |worker_thread_| should be created in SetUp() instead for symmetry but I don't feel strongly there, up to you) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:147: worker_thread_->JoinForTesting(); In fact I think TearDown() should do both: ExpectLastPostedTaskHasRun(); worker_thread_->JoinForTesting(); Though ExpectLastPostedTaskHasRun(); probably also still belongs in this test's body to make it clear what the test is verifying (the one in TearDown() merely being a double-check that all tests do wait and verify all their things before wind down). In that spirit TearDown() could also check that all BecomesIdleCallback invocations were observed (maybe loosening it up to having observing >= N-1 invocations to avoid forcing a check in tests that don't care about the last idle notification). i.e. I don't think this test currently catches a bug where too many notifications are fired and that's one way to. WDYT? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:203: new Sequence, &shared_priority_queue_, &task_tracker_); make_scoped_refptr(new Sequence) here and below (Chromium is moving towards making scoped_refptr(T*) explicit -- long story: https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/TlL1D-Djta0...) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:255: } A good chunk of ThreadMain is about handling race conditions between sleeping and receiving work. Can we come up with a test that stresses this? We probably can't in a deterministic way, but can we spam the queue somehow and make these conditions a possibility that would be caught sometimes if it's handled wrong? Flaky test failures are bad, but they're better than flaky product, so long as we can make failures actionable enough to get the bug fixed instead of the test disabled..!
https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... File base/task_scheduler/priority_queue.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... base/task_scheduler/priority_queue.cc:19: : sequence(sequence), sort_key(sort_key) {} On 2016/03/21 19:11:53, gab wrote: > std::move(sequence) > > (I know it already landed but just spotted this one while looking for more > instances of other missing moves I find in this file) > > Not sure if compiler can figure out by itself that this can be moved as it's not > referenced after..? scoped_ptr now points to std::unique_ptr which does not satisfy CopyConstructable, so I think the compiler figures it out.
https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... File base/task_scheduler/priority_queue.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... base/task_scheduler/priority_queue.cc:19: : sequence(sequence), sort_key(sort_key) {} On 2016/03/21 19:15:40, robliao wrote: > On 2016/03/21 19:11:53, gab wrote: > > std::move(sequence) > > > > (I know it already landed but just spotted this one while looking for more > > instances of other missing moves I find in this file) > > > > Not sure if compiler can figure out by itself that this can be moved as it's > not > > referenced after..? > > scoped_ptr now points to std::unique_ptr which does not satisfy > CopyConstructable, so I think the compiler figures it out. Yes, but here |sequence| is scoped_refptr. I'm not worried about scoped_ptr because either it works without move or it doesn't, whereas scoped_refptr can result in hidden thread-safe refbumps.
https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... File base/task_scheduler/priority_queue.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... base/task_scheduler/priority_queue.cc:19: : sequence(sequence), sort_key(sort_key) {} On 2016/03/21 19:55:19, gab wrote: > On 2016/03/21 19:15:40, robliao wrote: > > On 2016/03/21 19:11:53, gab wrote: > > > std::move(sequence) > > > > > > (I know it already landed but just spotted this one while looking for more > > > instances of other missing moves I find in this file) > > > > > > Not sure if compiler can figure out by itself that this can be moved as it's > > not > > > referenced after..? > > > > scoped_ptr now points to std::unique_ptr which does not satisfy > > CopyConstructable, so I think the compiler figures it out. > > Yes, but here |sequence| is scoped_refptr. > > I'm not worried about scoped_ptr because either it works without move or it > doesn't, whereas scoped_refptr can result in hidden thread-safe refbumps. Oops! My mistake. scoped_refptr is CopyConstructable so yes, we'll need std::move here to be explicit about moving. Yeah, I'm not sure if the move constructor is a legal optimization for variables no longer referenced after the transfer statement.
https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... File base/task_scheduler/priority_queue.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/pri... base/task_scheduler/priority_queue.cc:19: : sequence(sequence), sort_key(sort_key) {} On 2016/03/21 19:59:54, robliao wrote: > On 2016/03/21 19:55:19, gab wrote: > > On 2016/03/21 19:15:40, robliao wrote: > > > On 2016/03/21 19:11:53, gab wrote: > > > > std::move(sequence) > > > > > > > > (I know it already landed but just spotted this one while looking for more > > > > instances of other missing moves I find in this file) > > > > > > > > Not sure if compiler can figure out by itself that this can be moved as > it's > > > not > > > > referenced after..? > > > > > > scoped_ptr now points to std::unique_ptr which does not satisfy > > > CopyConstructable, so I think the compiler figures it out. > > > > Yes, but here |sequence| is scoped_refptr. > > > > I'm not worried about scoped_ptr because either it works without move or it > > doesn't, whereas scoped_refptr can result in hidden thread-safe refbumps. > > Oops! My mistake. scoped_refptr is CopyConstructable so yes, we'll need > std::move here to be explicit about moving. > > Yeah, I'm not sure if the move constructor is a legal optimization for variables > no longer referenced after the transfer statement. So far I'm reading this as move constructor optimizations are currently not allowed per overloading: http://en.cppreference.com/w/cpp/language/move_constructor If both copy and move constructors are provided, overload resolution selects the move constructor if the argument is an rvalue (either a prvalue such as a nameless temporary or an xvalue such as the result of std::move), and selects the copy constructor if the argument is an lvalue (named object or a function/operator returning lvalue reference). If only the copy constructor is provided, all argument categories select it (as long as it takes a reference to const, since rvalues can bind to const references), which makes copying the fallback for moving, when moving is unavailable.
gab@: All done. robliao@: Can you review this CL? gab@ is on vacation now :) https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:22: SequenceSortKey sequence_sort_key = sequence->GetSortKey(); On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > const Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:24: new PriorityQueue::SequenceAndSortKey(sequence, sequence_sort_key))); On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > std::move(sequence) > > Not sure whether compiler can figure out that this is movable as it's not > referenced after but moving it makes the intent explicit. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:34: DCHECK(task.get()); On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > No .get() for bool operations here and below Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.cc:39: task_tracker->PostTask(Bind(&PostTaskCallback, sequence, priority_queue), On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > std::move(sequence) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils.h:20: // If TaskTracker allows |task| to be posted, inserts it into |sequence|. Then, On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > s/TaskTracker/|task_tracker|/ Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... File base/task_scheduler/utils_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:31: // Post a task. On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > s/a/the/ Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:37: // Expect to find the task in the sequence. On 2016/03/21 19:11:53, gab (OOO until Apr. 5) wrote: > // Expect to find the task alone in the sequence. > ^^ > > (i.e. to highlight why you're testing a PopTask() after) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:63: // Expect to find the task in the sequence. On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > // Expect to find the task in the sequence behind the initial task. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/uti... base/task_scheduler/utils_unittest.cc:64: sequence->PopTask(); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > EXPECT_NE(task_raw, sequence->PeekTask()); > > before first pop. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:69: return true; On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > I think this can be implemented relatively easily in this by: > > 1) Having a scoped_ptr<ThreadCheckerImpl> member in WorkerThread which is > initialized on from ThreadMain(). > 2) Having this SchedulerSingleThreadTaskRunner task a non-null > ThreadCheckerImpl* in constructor. > 3) Verifying against it here. > > This is akin to how MessageLoopTaskRunner implements it [1], but I prefer using > ThreadCheckerImpl as it essentially does the same thing while hiding the > low-level details in a single member. > > Note: I'd add this to (1): > // A ThreadChecker to be used to verify that the current thread is this > // WorkerThread. Uses ThreadCheckerImpl instead of ThreadChecker to be able to > // support the non-dcheck only RunsTasksOnCurrentThread() SingleThreadTaskRunner > // API. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/... As discussed earlier, this will be implemented with a ThreadLocalPointer. Should I do it in this CL? https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:76: return PostDelayedTask(from_here, task, delay); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Add: > > // Tasks are never nested on WorkerThread. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:115: wake_up_cv_->Signal(); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > tl;dr; no explicit request here, mostly thoughts and looking for your opinion. > > I find this kind of hard to read without assumptions as nothing tells me > |wake_up_cv_| is a CV on |lock_| without double-checking the constructor. > > Not sure what to do about this though... comment below about using it as the > bool solves one side of the issue on the Wait() side where it is created, but > the Signal() side still comes out of the blue. > > I guess this is always a problem with CVs unless there'd be a wrapper to hold > both a lock and its corresponding CV. But if Chrome hasn't needed this yet > perhaps it's also okay for us to have them side-by-side. > > EDIT: Reading the test code now which uses multiple CVs for the same lock I see > how it makes sense to decouple the two. I guess it's implicit that the CV is > based on the only Lock in this class and then its name indicates what it's used > for. I now use a WaitableEvent instead of a ConditionVariable. waitable_event.h: "Use a WaitableEvent when you would otherwise use a Lock+ConditionVariable to protect a simple boolean value." https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:123: traits, &single_thread_priority_queue_, task_tracker_)); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > This is only fine because we never delete WorkerThreads outside tests (in which > no tasks are ever posted after they're deleted). > > Add a comment explaining why leaking raw pointers to members is okay. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:158: // Mac only supports 2 priorities. crbug.com/554651 On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Looks like this is fixed, I think we can remove the Mac only code here. Done. I pinged erikchen@ to have this landed. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:166: PlatformThread::CreateWithPriority(kDefaultStackSize, this, &thread_handle_, On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > In theory we'd need to use CreateNonJoinable() -- or introduce > CreateNonJoinableWithPriority() -- since we don't join our threads on shutdown > (right?). > > We do wait for work queues to be empty though. Looks like we need to understand > the impact of a non-joinable thread. > > AFAICT it only prevents use of singletons, which is probably too strict for the > way we handle things. That restrictions is probably merely so that a singleton > isn't in use while it gets destroyed (late on main thread by AtExitManager or > statically I think). In which case we could be fine enforcing ourselves that > singletons not be allowed in the scope of CONTINUE_ON_SHUTDOWN tasks, but be > okay on others. > > Today, the POSIX WorkerPool uses this, but SequencedWorkerPool doesn't -- which > is probably a bug (or at least a failure to check for potential bugs w.r.t. > CONTINUE_ON_SHUTDOWN). We don't want to use CreateNonJoinable() because: - We need to be able to join the thread in tests + I don't think we want a different behavior in test vs. prod. - It prevents the use of singletons on the thread. I think we should call SetSingletonAllowed(false) ourselves for CONTINUE_ON_SHUTDOWN tasks (maybe in TaskTracker::RunTask?). https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:189: (!shared_sequence.is_null() && On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > This check is redundant. If single_thread_sequence.is_null() then it's > impossible that shared_sequence.is_null() or the condition on line 184 would > have been true. No. If we remove !shared_sequence.is_null(), |shared_sequence| is null and |single_thread_sequence| is *not* null: - Condition at line 184 is false. - Condition at lines 188-190 is true... but should be false. Should I make sure that the |sort_key| of a null SequenceAndSortKey is less than any other sort key? You asked me to rely solely on is_null() instead of playing with std::numeric_limits in another review. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:204: wake_up_cv_->Wait(); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > So long as WaitUntilWakeUp() is only called from one thread, i.e. single waiter > (can enforce with the ThreadCheckerImpl proposed above). > > Then I think we can do the same trick we did in TaskTracker and use the CV's > existence as the bool. > > I guess it depends how often we think we'll need to re-create this CV and > whether that would result in heap churn (the Shutdown case in TaskTracker was an > easy case, this one is less clear I just tend to not like things that have more > or less the same state across two members). I now use a WaitableEvent. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:208: while (!should_exit_for_testing_) { On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Add a comment explaining what happens in product (i.e. run out of task, sleep, > and get killed by the OS on main exit) and why this is considered preferable to > joining all threads. I added !task_tracker_->shutdown_completed() so that the thread exits when shutdown completes. That way, it won't waste CPU to empty its priority queues from remaining CONTINUE/SKIP_ON_SHUTDOWN tasks (these tasks won't run, but with the existing code, the WorkerThread will waste time popping them from their pqs). https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:212: if (sequence.get() == nullptr) { On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > if (!sequence) > > (here and below) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:221: // |shared_priority_queue_| after the first call to GetWork() but before On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Could also have been added to |single_thread_priority_queue|, no? > > The way I understand this, this is: > > // Check one more time if there is work available. Work could have been added > // between the first call to GetWork() and the |becomes_idle_callback_| > // invocation in which case this WorkerThread shouldn't go to sleep. Done. This extra check is really just for |shared_priority_queue_| because |wake_up_event_| is always signaled when work is added to |single_thread_priority_queue_|. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:224: sequence = GetWork(&sequence_is_single_threaded); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > If we do get work here don't we need to self-wake, i.e. mark |is_awake_ = > true;|? Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:230: if (should_exit_for_testing_) On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Need to lock to check this member, right? > > Can we avoid having an AutoLock in product code for test-only code though? We no longer need this with a WaitableEvent. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:235: sequence = GetWork(&sequence_is_single_threaded); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Here I'd say "continue;" is easier to read, results in the same behavior but > makes one less piece in logic, i.e. "restart" instead of "re-get work and hope > to get something but we still might not". > > Also "continue;" allows us to get rid of the null check on line 239 :-) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:247: PushSequenceInPriorityQueue(std::move(sequence), On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > Inline this method here? It's a 2 lines helper and only used here. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:30: // Callback invoked by |worker_thread| to reinsert |sequence| in the On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > The callback contract can't really dictate who calls it. So maybe instead: > > // Callback invoked to reinsert |sequence| in the appropriate PriorityQueue > after one of its tasks was executed on |worker_thread|. > > And actually this comment should probably not mention "priority queue" either as > nothing forces the destination to be a PQ. > > Two options: > 1) Make the comment more generic. > 2) Define the type on PriorityQueue instead of here. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:31: // appropriate priority queue it has executed one of its tasks. On 2016/03/21 19:11:55, gab wrote: > missing "after" before "it"? Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:40: // a single-thread priority queue and from |shared_priority_queue|. On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > s/a single-thread priority queue/its single-thread priority queue/ Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:51: // A WorkerThread must only be destroyed after JoinForTesting() has returned. On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Hmmm, isn't the WorkerThread destroyed in the product where JoinForTesting() is > an illegal call? > > Or I guess we decided to never delete scheduler objects on product shutdown? If > that's the reason, this should be highlighted in this comment. Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:60: // scheduling tasks on this WorkerThread with |traits|. On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > s/tasks on this WorkerThread with |traits|/tasks with |traits| on this > WorkerThread/ > > (i.e. makes it clearer that |traits| apply to tasks not WorkerThread) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:79: scoped_refptr<Sequence> GetWork(bool* single_thread); On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > s/single_thread/is_single_thread/ Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:82: void WaitUntilWakeUp(); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > WaitUntilWokenUp or WaitForWakeUp? Done (method gone). https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.h:97: // True from the time WakeUp() is called to the time the WorkerThread doesn't On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > s/the WorkerThread/this WorkerThread/ > > (same below) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:55: void WaitUntilLastPostedTaskHasRun() { On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > s/HasRun/Ran/ > > (here and below) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:104: ran_tasks_in_wrong_order_ = true; On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > ADD_FAILURE here too instead of maintaining extra state (see below) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:110: void RunTaskThatShouldNotRun() { ran_task_that_should_not_run_ = true; } On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Instead of having extra state here and checking for it, this method's body could > simply be: > > { > ADD_FAILURE() << "Unexpected task execution."; > } > > which is equivalent to EXPECT_FALSE(ran_task_that_should_not_run()); after + > makes error report synchronous instead of after the fact :-) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:112: // Lock protecting |run_task_cv_|. On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Hmm, it does more than that it seems? Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:139: ASSERT_NE(nullptr, worker_thread_.get()); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > ASSERT_TRUE(worker_thread_); Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:140: WaitUntilNumBecomesIdleCallbackInvocations(1); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Move these two lines to constructor instead of at beginning of each test? Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:145: WaitUntilNumBecomesIdleCallbackInvocations(2); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Since WaitUntilNumBecomesIdleCallbackInvocations(1); will move to constructor, > it will be less obvious why this is "2". > > So maybe instead it should just be WaitForNextBecomesIdleCallbackInvocation(); ? With WaitForNextBecomesIdleCallbackInvocation(), it's harder to handle the case where the callback invocation occurs before we wait for it. Anyways, the unit tests changed a lot and probably need a new review. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:147: worker_thread_->JoinForTesting(); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > Since the thread is created by the fixture I think JoinForTesting() belongs in > the fixture's TearDown(). > > (and maybe |worker_thread_| should be created in SetUp() instead for symmetry > but I don't feel strongly there, up to you) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:147: worker_thread_->JoinForTesting(); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > In fact I think TearDown() should do both: > > ExpectLastPostedTaskHasRun(); > worker_thread_->JoinForTesting(); > > Though ExpectLastPostedTaskHasRun(); probably also still belongs in this test's > body to make it clear what the test is verifying (the one in TearDown() merely > being a double-check that all tests do wait and verify all their things before > wind down). > > In that spirit TearDown() could also check that all BecomesIdleCallback > invocations were observed (maybe loosening it up to having observing >= N-1 > invocations to avoid forcing a check in tests that don't care about the last > idle notification). > i.e. I don't think this test currently catches a bug where too many > notifications are fired and that's one way to. > > WDYT? ExpectLastPostedTaskHasRun() is not done in the destructor of TaskClosureFactory. I had to create a factory for TaskSchedulerWorkerThreadTest.PostSingleThreadedTasksFromMultipleThreads. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:203: new Sequence, &shared_priority_queue_, &task_tracker_); On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > make_scoped_refptr(new Sequence) > > here and below (Chromium is moving towards making scoped_refptr(T*) explicit -- > long story: > https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/TlL1D-Djta0...) Done. https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread_unittest.cc:255: } On 2016/03/21 19:11:55, gab (OOO until Apr. 5) wrote: > A good chunk of ThreadMain is about handling race conditions between sleeping > and receiving work. > > Can we come up with a test that stresses this? We probably can't in a > deterministic way, but can we spam the queue somehow and make these conditions a > possibility that would be caught sometimes if it's handled wrong? Flaky test > failures are bad, but they're better than flaky product, so long as we can make > failures actionable enough to get the bug fixed instead of the test disabled..! Done.
On the whole, I think I like the new scheme better. Some comments: https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.cc:21: // |sequence| must be inserted in |priority_queue|. I would reword the last sentence as... Empty sequences are not part of any priority_queue, so it must be inserted. We would normally DCHECK for this, but I don't see an easy non-invasive way of scanning |priority_queue| without poking some holes and grabbing some locks. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.h:20: // If |task_tracker| allows |task| to be posted, inserts it into |sequence|. Nit: PostTaskHelper inserts it into |sequence|. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.h:20: // If |task_tracker| allows |task| to be posted, inserts it into |sequence|. Nit: This is restating the implementation. Perhaps you could go with something like... Helper for posting |task| to the provided |sequence| and |priority_queue| conditional on the |task_tracker|. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:148: const size_t kDefaultStackSize = 0; Might as well make this static const. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:154: DCHECK(is_single_threaded); I think this is one of the rare cases where a DCHECK might actually not be necessary given that we're writing to it on the next line. I leave it up to you. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:201: // hadn't be notified that this WorkerThread was idle and might have Nit: hasn't https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:220: wake_up_event_.Reset(); Why is this reset necessary since our events are currently autoreset? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:228: // Pop a task from |sequence|. If it is empty after this, reinsert it in I think you mean if the sequence isn't empty. I would reword the comment as follows... To maintain the non-empty sequence invariant for the PQ reinsert the sequence into its original PQ if it's not empty. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:56: // its single-threaded PriorityQueue and from |shared_priority_queue|. Nit: |single_thread_priority_queue_| and from |shared_priority_queue_| https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:70: // Destroying a WorkerThread in production is not allowed. In tests, it can How are we planning on reclaiming WorkerThreads in the future? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:29: const size_t kNumTasksPerTest = 500; Why 500? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:31: class TaskClosureFactory { Optional: Maybe SequencedTaskClosureFactory? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:47: void WaitUntilLastFactoredTaskRan() { WaitForAllTasksToRun https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:49: while (num_factored_tasks_ != num_run_tasks_) Nit: num_run_tasks_ < num_created_tasks_ Makes it easier to see the expected relationship between the two values. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:57: if (task_index != num_run_tasks_ + 1) If you allow task_index to be zero, you can avoid the + 1 here. Make the bind use num_created_tasks_++ https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:71: size_t num_factored_tasks_ = 0; num_created_tasks_ https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:110: virtual void SetUp() override { Nit: virtual not needed with override. Here and below. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:136: while (num_state_changes_ != expected_num_state_changes) Nit: num_state_changes_ < expected_num_state_changes https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:199: // Verify each call to WakeUp() on an idle WorkerThread causes 2 state changes. This seems like a brittle test, especially if for some reason we need more tests. It may be more helpful to verify that the states go through an expected state ordering instead (e.g. from IDLE to BUSY and then back to IDLE) https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:203: WaitUntilNumStateChanges(2 * (i + 1)); Should this instead WaitUntilIdle? If so, you can remove WaitUntilNumStateChanges https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:223: // Verify that 500 tasks run successfully when they are posted through a task Maybe |kNumTasksPerTest| instead of 500. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:243: TEST_F(TaskSchedulerWorkerThreadTest, Isn't this similar to PostOneSingleThreadedTask, just called kNumTasksPerTest? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:269: task_runner_b->PostTask(FROM_HERE, factory.CreateTaskClosure()); Do we guarantee task execution ordering across task runners? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:280: // task to the priority queue. Mention that the wakeup is necessary since shared tasks do not automatically wake up the thread. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:297: // the WorkerThread each time it adds a task to the priority queue. The worker thread looks like it's only woken up once. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:369: sequence_a, &shared_priority_queue_, &task_tracker_); We definitely don't guarantee interleaving across sequences. This should probably relax the run check.
Description was changed from ========== TaskScheduler [6/9] WorkerThread This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6/9] WorkerThread A WorkerThread that runs Tasks from a shared and a single-threaded PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
Description was changed from ========== TaskScheduler [6/9] WorkerThread A WorkerThread that runs Tasks from a shared and a single-threaded PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6/9] WorkerThread A WorkerThread runs Tasks from a shared and a single-threaded PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
robliao@: I addressed all your comments. Note that the tests sometimes fail because posting a task to a single-thread task runner can cause 4 state changes (tests expect 2 state changes). This problem will be solved once I upload a new patch that doesn't require a second call to GetWork() before going to sleep. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... File base/task_scheduler/utils.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.cc:21: // |sequence| must be inserted in |priority_queue|. On 2016/03/24 23:03:26, robliao wrote: > I would reword the last sentence as... > > Empty sequences are not part of any priority_queue, so it must be inserted. We > would normally DCHECK for this, but I don't see an easy non-invasive way of > scanning |priority_queue| without poking some holes and grabbing some locks. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... File base/task_scheduler/utils.h (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.h:20: // If |task_tracker| allows |task| to be posted, inserts it into |sequence|. On 2016/03/24 23:03:26, robliao wrote: > Nit: This is restating the implementation. Perhaps you could go with something > like... > Helper for posting |task| to the provided |sequence| and |priority_queue| > conditional on the |task_tracker|. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/ut... base/task_scheduler/utils.h:20: // If |task_tracker| allows |task| to be posted, inserts it into |sequence|. On 2016/03/24 23:03:26, robliao wrote: > Nit: PostTaskHelper inserts it into |sequence|. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:148: const size_t kDefaultStackSize = 0; On 2016/03/24 23:03:26, robliao wrote: > Might as well make this static const. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:154: DCHECK(is_single_threaded); On 2016/03/24 23:03:26, robliao wrote: > I think this is one of the rare cases where a DCHECK might actually not be > necessary given that we're writing to it on the next line. I leave it up to you. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:201: // hadn't be notified that this WorkerThread was idle and might have On 2016/03/24 23:03:26, robliao wrote: > Nit: hasn't I don't think that "hasn't" is correct because at this point, the recipient HAS been notified. I reworded the sentence to express that it hadn't be notified when the work was posted. Is is clearer? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:220: wake_up_event_.Reset(); On 2016/03/24 23:03:26, robliao wrote: > Why is this reset necessary since our events are currently autoreset? I moved the reset at the bottom of this method and added a comment to explain why it's there. I think it's a nice optimization but it's not absolutely required. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:228: // Pop a task from |sequence|. If it is empty after this, reinsert it in On 2016/03/24 23:03:26, robliao wrote: > I think you mean if the sequence isn't empty. > > I would reword the comment as follows... > To maintain the non-empty sequence invariant for the PQ reinsert the sequence > into its original PQ if it's not empty. We don't necessary reinsert the sequence in its original PQ. A sequence with BACKGROUND+USER_VISIBLE tasks can change priority queue when its USER_VISIBLE task is popped. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:56: // its single-threaded PriorityQueue and from |shared_priority_queue|. On 2016/03/24 23:03:26, robliao wrote: > Nit: |single_thread_priority_queue_| and from |shared_priority_queue_| The intent of this comment is to describe the arguments of CreateWorkerThread(), not the private members of the class. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:70: // Destroying a WorkerThread in production is not allowed. In tests, it can On 2016/03/24 23:03:26, robliao wrote: > How are we planning on reclaiming WorkerThreads in the future? Once a WorkerThread has been created in production, it cannot be reclaimed. A ThreadPool creates a given number of WorkerThreads when it is created and can rely on them forever. We should compare the cost of (1) keeping threads alive vs. the cost of (2) exiting and recreating threads on demand. If we determine that (2) is better, I still prefer to have a list of WorkerThread that never go away in ThreadPool. However, each WorkerThread could manage the lifetime of its platform thread (e.g. the platform thread exits after 30 seconds of inactivity and is recreated when WakeUp is called). I think that this solution will lead to simpler code than a ramping up/down algorithm in ThreadPool. WDYT? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:29: const size_t kNumTasksPerTest = 500; On 2016/03/24 23:03:27, robliao wrote: > Why 500? A high number of tasks increases the chances to catch bugs (I discovered+fixed bugs that occurred only after 100 iterations when I wrote these tests). No particular reason for choosing 500. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:31: class TaskClosureFactory { On 2016/03/24 23:03:26, robliao wrote: > Optional: Maybe SequencedTaskClosureFactory? Now I use the factory to create non-sequenced tasks... https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:47: void WaitUntilLastFactoredTaskRan() { On 2016/03/24 23:03:26, robliao wrote: > WaitForAllTasksToRun Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:49: while (num_factored_tasks_ != num_run_tasks_) On 2016/03/24 23:03:26, robliao wrote: > Nit: num_run_tasks_ < num_created_tasks_ > Makes it easier to see the expected relationship between the two values. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:57: if (task_index != num_run_tasks_ + 1) On 2016/03/24 23:03:27, robliao wrote: > If you allow task_index to be zero, you can avoid the + 1 here. > > Make the bind use num_created_tasks_++ Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:71: size_t num_factored_tasks_ = 0; On 2016/03/24 23:03:26, robliao wrote: > num_created_tasks_ Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:110: virtual void SetUp() override { On 2016/03/24 23:03:26, robliao wrote: > Nit: virtual not needed with override. > Here and below. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:136: while (num_state_changes_ != expected_num_state_changes) On 2016/03/24 23:03:27, robliao wrote: > Nit: num_state_changes_ < expected_num_state_changes Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:199: // Verify each call to WakeUp() on an idle WorkerThread causes 2 state changes. On 2016/03/24 23:03:26, robliao wrote: > This seems like a brittle test, especially if for some reason we need more > tests. It may be more helpful to verify that the states go through an expected > state ordering instead (e.g. from IDLE to BUSY and then back to IDLE) StateChangedCallback() verifies that the states go through an expected state ordering. I added a comment to highlight that fact. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:203: WaitUntilNumStateChanges(2 * (i + 1)); On 2016/03/24 23:03:26, robliao wrote: > Should this instead WaitUntilIdle? If so, you can remove > WaitUntilNumStateChanges Replacing WaitUntilNumStateChanges() with WaitUntilIdle() isn't correct because it doesn't verify that the thread has gone through the BUSY state since it was woken up (the test could pass if the state is always IDLE). https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:223: // Verify that 500 tasks run successfully when they are posted through a task On 2016/03/24 23:03:27, robliao wrote: > Maybe |kNumTasksPerTest| instead of 500. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:243: TEST_F(TaskSchedulerWorkerThreadTest, On 2016/03/24 23:03:27, robliao wrote: > Isn't this similar to PostOneSingleThreadedTask, just called kNumTasksPerTest? Yes, PostOneSingleThreadedTask is equivalent to this test with 1 loop iteration. I had both tests because PostOneSingleThreadedTask is easier to debug and PostMultipleSingleThreadedTasksWaitBetweenPosts is more likely to catch synchronization errors. In the latest patch set, I removed PostOneSingleThreadedTask to reduce the amount of code to review. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:269: task_runner_b->PostTask(FROM_HERE, factory.CreateTaskClosure()); On 2016/03/24 23:03:26, robliao wrote: > Do we guarantee task execution ordering across task runners? The current implementation runs tasks posted to the same WorkerThread / same priority in posting order. However, we might want to change that in the future (e.g. if we realize that there is less contention on the pq lock if we run tasks from the same sequence in batch). For that reason, I now have a different factory for each task runner. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:280: // task to the priority queue. On 2016/03/24 23:03:27, robliao wrote: > Mention that the wakeup is necessary since shared tasks do not automatically > wake up the thread. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:297: // the WorkerThread each time it adds a task to the priority queue. On 2016/03/24 23:03:27, robliao wrote: > The worker thread looks like it's only woken up once. Done. https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:369: sequence_a, &shared_priority_queue_, &task_tracker_); On 2016/03/24 23:03:27, robliao wrote: > We definitely don't guarantee interleaving across sequences. This should > probably relax the run check. Done.
robliao@: Can you review this CL? I plan to use the shared priority queue's lock to synchronize accesses to the stack of IDLE worker threads. I didn't find any bug in the previous implementation. However, I prefer this implementation because: - No need to explain why we need to call GetWork() a second time before going to sleep. - The number of state change notifications is predictable.
https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:201: // hadn't be notified that this WorkerThread was idle and might have On 2016/03/29 18:33:34, fdoray wrote: > On 2016/03/24 23:03:26, robliao wrote: > > Nit: hasn't > > I don't think that "hasn't" is correct because at this point, the recipient HAS > been notified. I reworded the sentence to express that it hadn't be notified > when the work was posted. Is is clearer? This CL comment is no longer relevant as of patchset 12 https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:70: // Destroying a WorkerThread in production is not allowed. In tests, it can On 2016/03/29 18:33:34, fdoray wrote: > On 2016/03/24 23:03:26, robliao wrote: > > How are we planning on reclaiming WorkerThreads in the future? > > Once a WorkerThread has been created in production, it cannot be reclaimed. A > ThreadPool creates a given number of WorkerThreads when it is created and can > rely on them forever. > > We should compare the cost of (1) keeping threads alive vs. the cost of (2) > exiting and recreating threads on demand. If we determine that (2) is better, I > still prefer to have a list of WorkerThread that never go away in ThreadPool. > However, each WorkerThread could manage the lifetime of its platform thread > (e.g. the platform thread exits after 30 seconds of inactivity and is recreated > when WakeUp is called). I think that this solution will lead to simpler code > than a ramping up/down algorithm in ThreadPool. WDYT? I think this will be hard to determine until we actually do (2). Since we're not ramping up or down yet, we can stick with this, but it's certainly something to keep in mind. https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:92: DCHECK(is_single_threaded); Done as in DCHECK necessary or DCHECK unnecessary? https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:191: state_changed_callback_.Run(this, state); I'm not sure I like this callback as we're holding a lock right before we call it. We really should release the lock before this point since now it's now very easy the caller to do the wrong thing and say, do something with the shared priority queue. Another solution I haven't completely fleshed out, but might have promise, is having the code to get more work happen at the ThreadPool level. In this world, WorkerThread::ThreadMain calls back to the ThreadPool to request more work. The ThreadPool can decide if the worker thread should do more work (return a sequence) or go to sleep (add to the idle list, and then return null). Since getting the work is delegated, the threadpool can update its the idle list before the thread goes to sleep, similar to this. This allows the locks to live at the ThreadPool level and removes the tension here of updating the idle list owned by the threadpool and going to sleep. I haven't completely fleshed out all the details, but feel free to tell me if this will completely break something :-). https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:217: wake_up_event_.Wait(); Between shared_transaction.reset and wake_up_event.Wait() seems like a potentially problematic point. It might be worth documenting that nothing should occur between these two statements.
https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:191: state_changed_callback_.Run(this, state); On 2016/03/30 00:42:42, robliao wrote: > I'm not sure I like this callback as we're holding a lock right before we call > it. > We really should release the lock before this point since now it's now very easy > the caller to do the wrong thing and say, do something with the shared priority > queue. > > Another solution I haven't completely fleshed out, but might have promise, is > having the code to get more work happen at the ThreadPool level. > > In this world, WorkerThread::ThreadMain calls back to the ThreadPool to request > more work. The ThreadPool can decide if the worker thread should do more work > (return a sequence) or go to sleep (add to the idle list, and then return null). > > > Since getting the work is delegated, the threadpool can update its the idle list > before the thread goes to sleep, similar to this. > > This allows the locks to live at the ThreadPool level and removes the tension > here of updating the idle list owned by the threadpool and going to sleep. > > I haven't completely fleshed out all the details, but feel free to tell me if > this will completely break something :-). This also means that the threadpool will have to make decisions regarding taking work from the shared queue or the single-threaded queue, which might be good in that it allows us to centralize the execution policy in threadpool.
Description was changed from ========== TaskScheduler [6/9] WorkerThread A WorkerThread runs Tasks from a shared and a single-threaded PriorityQueue. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6/9] WorkerThread A WorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
robliao@: Can you take another look? I like the idea of using a callback to get work. It will allow us to have a single method in ThreadPool to choose what work a WorkerThread should execute + add/remove it from a stack of idle threads depending on whether work is returned. https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:92: DCHECK(is_single_threaded); On 2016/03/30 00:42:42, robliao wrote: > Done as in DCHECK necessary or DCHECK unnecessary? > > https://codereview.chromium.org/1704113002/diff/120001/base/task_scheduler/wo... I removed it in patch set 11... and I re-added it in patch set 12 to be consistent with the other arguments. No longer relevant in patch set 13. https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:191: state_changed_callback_.Run(this, state); On 2016/03/30 00:47:28, robliao wrote: > On 2016/03/30 00:42:42, robliao wrote: > > I'm not sure I like this callback as we're holding a lock right before we call > > it. > > We really should release the lock before this point since now it's now very > easy > > the caller to do the wrong thing and say, do something with the shared > priority > > queue. > > > > Another solution I haven't completely fleshed out, but might have promise, is > > having the code to get more work happen at the ThreadPool level. > > > > In this world, WorkerThread::ThreadMain calls back to the ThreadPool to > request > > more work. The ThreadPool can decide if the worker thread should do more work > > (return a sequence) or go to sleep (add to the idle list, and then return > null). > > > > > > Since getting the work is delegated, the threadpool can update its the idle > list > > before the thread goes to sleep, similar to this. > > > > This allows the locks to live at the ThreadPool level and removes the tension > > here of updating the idle list owned by the threadpool and going to sleep. > > > > I haven't completely fleshed out all the details, but feel free to tell me if > > this will completely break something :-). > > This also means that the threadpool will have to make decisions regarding taking > work from the shared queue or the single-threaded queue, which might be good in > that it allows us to centralize the execution policy in threadpool. I like the idea of delegating GetWork() to the ThreadPool. So far, WorkerThread always had some knowledge of the stack of idle threads (e.g. check one more time if the pq are empty in case work was added before the WorkerThread was pushed on the stack of idle WorkerThreads). With a callback, this stack becomes perfectly encapsulated in ThreadPool. https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:217: wake_up_event_.Wait(); On 2016/03/30 00:42:42, robliao wrote: > Between shared_transaction.reset and wake_up_event.Wait() seems like a > potentially problematic point. It might be worth documenting that nothing should > occur between these two statements. > Why is it problematic? What shouldn't happen between these 2 statements? Calling WakeUp() guarantees that the WorkerThread will stay awake until it has no more work left. This is true with this code because we always check if there is work left between wake_up_event_.Reset() and wake_up_event_.Wait().
Much simpler! :-) https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:217: wake_up_event_.Wait(); On 2016/03/30 18:44:49, fdoray wrote: > On 2016/03/30 00:42:42, robliao wrote: > > Between shared_transaction.reset and wake_up_event.Wait() seems like a > > potentially problematic point. It might be worth documenting that nothing > should > > occur between these two statements. > > > > Why is it problematic? What shouldn't happen between these 2 statements? > > Calling WakeUp() guarantees that the WorkerThread will stay awake until it has > no more work left. This is true with this code because we always check if there > is work left between wake_up_event_.Reset() and wake_up_event_.Wait(). Previously, we had a CV do the work here so we could release the lock and sleep atomically. Now we have two different statements. As structured, the code looks like it works correctly, but we'll have to remember to avoid depending on any state between these two lines. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:65: // Wait for a wake-up. Instead, consider commenting that all WorkerThreads start out sleeping. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:73: // Wait for a wake-up. Nit: Remove this comment. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:78: // Peek the next task in |sequence| and try to run it. Nit: This comment can be removed here as it just restates the code below. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:82: // run a Task from |sequence|. Same here https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:85: // Calling WakeUp() guarantees that this WorkerThread will run Tasks from This comment is worth keeping as it explains the reason this statement is here. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:35: using GetWorkCallback = At some point it might make sense to have a WorkerThreadDelegate interface instead since these are going to be base::bind(..., base::Unretained(ThreadPool)). class WorkerThreadDelegate { scoped_refptr<Sequence> GetWork(const WorkerThread* worker_thread) = 0; void FinishedWork(const WorkerThread* worker_thread, scoped_refptr<Sequence> sequence) = 0; };
robliao@: Can you take another look? https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/220001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:217: wake_up_event_.Wait(); On 2016/03/30 19:38:26, robliao wrote: > On 2016/03/30 18:44:49, fdoray wrote: > > On 2016/03/30 00:42:42, robliao wrote: > > > Between shared_transaction.reset and wake_up_event.Wait() seems like a > > > potentially problematic point. It might be worth documenting that nothing > > should > > > occur between these two statements. > > > > > > > Why is it problematic? What shouldn't happen between these 2 statements? > > > > Calling WakeUp() guarantees that the WorkerThread will stay awake until it has > > no more work left. This is true with this code because we always check if > there > > is work left between wake_up_event_.Reset() and wake_up_event_.Wait(). > > Previously, we had a CV do the work here so we could release the lock and sleep > atomically. Now we have two different statements. As structured, the code looks > like it works correctly, but we'll have to remember to avoid depending on any > state between these two lines. ok, I agree. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:65: // Wait for a wake-up. On 2016/03/30 19:38:26, robliao wrote: > Instead, consider commenting that all WorkerThreads start out sleeping. Done. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:73: // Wait for a wake-up. On 2016/03/30 19:38:26, robliao wrote: > Nit: Remove this comment. Done. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:78: // Peek the next task in |sequence| and try to run it. On 2016/03/30 19:38:27, robliao wrote: > Nit: This comment can be removed here as it just restates the code below. Done. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:82: // run a Task from |sequence|. On 2016/03/30 19:38:26, robliao wrote: > Same here Done. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:85: // Calling WakeUp() guarantees that this WorkerThread will run Tasks from On 2016/03/30 19:38:27, robliao wrote: > This comment is worth keeping as it explains the reason this statement is here. Acknowledged. https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:35: using GetWorkCallback = On 2016/03/30 19:38:27, robliao wrote: > At some point it might make sense to have a WorkerThreadDelegate interface > instead since these are going to be base::bind(..., > base::Unretained(ThreadPool)). > > class WorkerThreadDelegate { > scoped_refptr<Sequence> GetWork(const WorkerThread* worker_thread) = 0; > void FinishedWork(const WorkerThread* worker_thread, > scoped_refptr<Sequence> sequence) = 0; > }; In fact, the target of GetWorkCallback is a ThreadPool (shared+single-threaded PriorityQueues will be owned by the WorkerThread's ThreadPool) and the target of RanTaskFromSequenceCallback is the TaskScheduler (because a Sequence can be reinserted in a PriorityQueue that belongs to another ThreadPool). I would have used a delegate if multiple methods were called on the same target, but here I prefer callbacks. WDYT?
lgtm https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/260001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:35: using GetWorkCallback = On 2016/03/30 19:56:37, fdoray wrote: > On 2016/03/30 19:38:27, robliao wrote: > > At some point it might make sense to have a WorkerThreadDelegate interface > > instead since these are going to be base::bind(..., > > base::Unretained(ThreadPool)). > > > > class WorkerThreadDelegate { > > scoped_refptr<Sequence> GetWork(const WorkerThread* worker_thread) = 0; > > void FinishedWork(const WorkerThread* worker_thread, > > scoped_refptr<Sequence> sequence) = 0; > > }; > > In fact, the target of GetWorkCallback is a ThreadPool (shared+single-threaded > PriorityQueues will be owned by the WorkerThread's ThreadPool) and the target of > RanTaskFromSequenceCallback is the TaskScheduler (because a Sequence can be > reinserted in a PriorityQueue that belongs to another ThreadPool). I would have > used a delegate if multiple methods were called on the same target, but here I > prefer callbacks. WDYT? sgtm.
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Can you review this CL? Thanks.
https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:27: return scoped_ptr<WorkerThread>(); return nullptr https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:31: class BASE_EXPORT WorkerThread : public PlatformThread::Delegate { Did you know there's already a class named WorkerThread in base (inside a test only) https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor... As well as quite a number of other "WorkerThread" classes in chrome. https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20(.*%5C%20)... Perhaps this could have a more unique name? SchedulerWorkerThread maybe? I'm not very creative with names. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:49: static scoped_ptr<WorkerThread> CreateWorkerThread( When can this return null? Please document. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:80: base::subtle::MemoryBarrier(); uhmm.. I never expected to see this. :P Why aren't you just using Lock? Maybe you want some #ifdef UNIT_TEST? Or, volatile? https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:73: void set_num_sequences_to_create(size_t num_sequences_to_create) { nit: SetNumSequencesToCreate. this isn't just a simple setter. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:79: size_t num_get_work_callback() const { nitto, NumGetWorkCallback, locking make this not "cheap" IMO https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:93: AutoSchedulerLock auto_lock(lock_); Am I imagining deadlocks here? WaitForNumGetWorkCallback() would hold this lock and wait on num_get_work_callback_cv_. This can't Signal, until this lock is released. Ditto for WaitForAllSequencesToRun and run_sequences_cv_? https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:101: return scoped_refptr<Sequence>(); return nullptr; https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:200: // Let the WorkerThread go to sleep. ? This looks like a flakey way to avoid races, but I'm not sure what you're trying to actual wait for to happen here.
danakj@: Thanks for the review. Can you take another look? https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.cc:27: return scoped_ptr<WorkerThread>(); On 2016/03/31 00:25:13, danakj wrote: > return nullptr Done. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:31: class BASE_EXPORT WorkerThread : public PlatformThread::Delegate { On 2016/03/31 00:25:13, danakj wrote: > Did you know there's already a class named WorkerThread in base (inside a test > only) > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor... > > As well as quite a number of other "WorkerThread" classes in chrome. > https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20(.*%5C%20)... > > Perhaps this could have a more unique name? SchedulerWorkerThread maybe? I'm not > very creative with names. Done. Renamed to SchedulerWorkerThread. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:49: static scoped_ptr<WorkerThread> CreateWorkerThread( On 2016/03/31 00:25:13, danakj wrote: > When can this return null? Please document. Done. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:80: base::subtle::MemoryBarrier(); On 2016/03/31 00:25:13, danakj wrote: > uhmm.. I never expected to see this. :P Why aren't you just using Lock? Maybe > you want some #ifdef UNIT_TEST? > > Or, volatile? I wanted to avoid taking a lock in production code to check a flag that is only used in tests. #ifdef UNIT_TEST doesn't work because UNIT_TEST is only defined for test source files (i.e. it is never defined when worker_thread.cc is compiled). volatile doesn't work because it doesn't provide memory ordering. I couldn't use it to guarantee that |should_exit_for_testing_| has been written before JoinForTesting() signals |wake_up_event_| [1] or to guarantee that |should_exit_for_testing_| is read at the expected time in WorkerThread::ThreadMain [2]. I changed the code to use a lock. There won't be contention on this lock in production code so it shouldn't be to costly... [1] Actually, I believe that signaling |wake_up_event_| acts like a memory barrier. However, I prefer not to rely on the side effects of signaling an event. [2] task_tracker_->shutdown_completed() acts like a memory barrier because it acquires a lock. Again, I prefer not to rely on this side effect. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:73: void set_num_sequences_to_create(size_t num_sequences_to_create) { On 2016/03/31 00:25:14, danakj wrote: > nit: SetNumSequencesToCreate. this isn't just a simple setter. Done. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:79: size_t num_get_work_callback() const { On 2016/03/31 00:25:13, danakj wrote: > nitto, NumGetWorkCallback, locking make this not "cheap" IMO Done. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:93: AutoSchedulerLock auto_lock(lock_); On 2016/03/31 00:25:13, danakj wrote: > Am I imagining deadlocks here? WaitForNumGetWorkCallback() would hold this lock > and wait on num_get_work_callback_cv_. This can't Signal, until this lock is > released. > > Ditto for WaitForAllSequencesToRun and run_sequences_cv_? It is a requirement of ConditionVariable to acquire its associated lock before calling Wait or Signal. Wait() releases the lock as it starts to sleep [1]. That means that the Signal() at line 97 can happen while WaitForNumGetWorkCallback() is waiting. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:101: return scoped_refptr<Sequence>(); On 2016/03/31 00:25:13, danakj wrote: > return nullptr; Done. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:200: // Let the WorkerThread go to sleep. On 2016/03/31 00:25:13, danakj wrote: > ? > > This looks like a flakey way to avoid races, but I'm not sure what you're trying > to actual wait for to happen here. Removed it. The goal was to give enough time to WorkerThread to call wake_up_event_.Wait() before going to the next iteration of this test. However, it's not absolutely necessary to wait for that (wake_up_event_.Wait() will return immediately if it is called before line 197 of this test runs).
https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:31: class BASE_EXPORT WorkerThread : public PlatformThread::Delegate { On 2016/03/31 03:26:31, fdoray wrote: > On 2016/03/31 00:25:13, danakj wrote: > > Did you know there's already a class named WorkerThread in base (inside a test > > only) > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor... > > > > As well as quite a number of other "WorkerThread" classes in chrome. > > > https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20(.*%5C%20)... > > > > Perhaps this could have a more unique name? SchedulerWorkerThread maybe? I'm > not > > very creative with names. > > Done. Renamed to SchedulerWorkerThread. Another solution would be to have a base::task_scheduler:: namespace. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:93: AutoSchedulerLock auto_lock(lock_); On 2016/03/31 03:26:31, fdoray wrote: > On 2016/03/31 00:25:13, danakj wrote: > > Am I imagining deadlocks here? WaitForNumGetWorkCallback() would hold this > lock > > and wait on num_get_work_callback_cv_. This can't Signal, until this lock is > > released. > > > > Ditto for WaitForAllSequencesToRun and run_sequences_cv_? > > It is a requirement of ConditionVariable to acquire its associated lock before > calling Wait or Signal. Wait() releases the lock as it starts to sleep [1]. That > means that the Signal() at line 97 can happen while WaitForNumGetWorkCallback() > is waiting. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... In fact, holding the ConditionVariable's lock is only a requirement for calling Wait (not Signal).
Description was changed from ========== TaskScheduler [6/9] WorkerThread A WorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6] WorkerThread A WorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
Description was changed from ========== TaskScheduler [6] WorkerThread A WorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6] WorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
Description was changed from ========== TaskScheduler [6] WorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6] SchedulerWorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
LGTM https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread.h:31: class BASE_EXPORT WorkerThread : public PlatformThread::Delegate { On 2016/03/31 13:29:24, fdoray wrote: > On 2016/03/31 03:26:31, fdoray wrote: > > On 2016/03/31 00:25:13, danakj wrote: > > > Did you know there's already a class named WorkerThread in base (inside a > test > > > only) > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/wor... > > > > > > As well as quite a number of other "WorkerThread" classes in chrome. > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=class%5C%20(.*%5C%20)... > > > > > > Perhaps this could have a more unique name? SchedulerWorkerThread maybe? I'm > > not > > > very creative with names. > > > > Done. Renamed to SchedulerWorkerThread. > > Another solution would be to have a base::task_scheduler:: namespace. I prefer changing the class name as you did. https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... File base/task_scheduler/worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/280001/base/task_scheduler/wo... base/task_scheduler/worker_thread_unittest.cc:93: AutoSchedulerLock auto_lock(lock_); On 2016/03/31 13:29:24, fdoray wrote: > On 2016/03/31 03:26:31, fdoray wrote: > > On 2016/03/31 00:25:13, danakj wrote: > > > Am I imagining deadlocks here? WaitForNumGetWorkCallback() would hold this > > lock > > > and wait on num_get_work_callback_cv_. This can't Signal, until this lock is > > > released. > > > > > > Ditto for WaitForAllSequencesToRun and run_sequences_cv_? > > > > It is a requirement of ConditionVariable to acquire its associated lock before > > calling Wait or Signal. Wait() releases the lock as it starts to sleep [1]. > That > > means that the Signal() at line 97 can happen while > WaitForNumGetWorkCallback() > > is waiting. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati... > > In fact, holding the ConditionVariable's lock is only a requirement for calling > Wait (not Signal). oh.. right I was missing that the CV is tied to that |lock_|, thanks.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org Link to the patchset: https://codereview.chromium.org/1704113002/#ps320002 (title: "add main entry callback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1704113002/320002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1704113002/320002
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [6] SchedulerWorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6] SchedulerWorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #18 (id:320002)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler [6] SchedulerWorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org ========== to ========== TaskScheduler [6] SchedulerWorkerThread A SchedulerWorkerThread runs Tasks from Sequences returned by a custom callback. This change is a subset of https://codereview.chromium.org/1698183005/ BUG=553459 CC_FUTURE=chromium-reviews, vmpstr+watch@chromium.org Committed: https://crrev.com/93649e8fc3faf88cee5a9b03b67199c4bb614b1c Cr-Commit-Position: refs/heads/master@{#384441} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/93649e8fc3faf88cee5a9b03b67199c4bb614b1c Cr-Commit-Position: refs/heads/master@{#384441}
Message was sent while issue was closed.
Did a full scan to catch up on where we are now after my vacation, this code is a thing of beauty :-), a few post-commit comments below though. Cheers! Gab https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... File base/task_scheduler/worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/80001/base/task_scheduler/wor... base/task_scheduler/worker_thread.cc:189: (!shared_sequence.is_null() && On 2016/03/24 19:21:10, fdoray wrote: > On 2016/03/21 19:11:54, gab (OOO until Apr. 5) wrote: > > This check is redundant. If single_thread_sequence.is_null() then it's > > impossible that shared_sequence.is_null() or the condition on line 184 would > > have been true. > > No. If we remove !shared_sequence.is_null(), |shared_sequence| is null and > |single_thread_sequence| is *not* null: > - Condition at line 184 is false. > - Condition at lines 188-190 is true... but should be false. > > Should I make sure that the |sort_key| of a null SequenceAndSortKey is less than > any other sort key? You asked me to rely solely on is_null() instead of playing > with std::numeric_limits in another review. True, my bad, this is fine as-is nvm. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:38: wake_up_event_.Signal(); Should this DCHECK(!wake_up_event_.IsSignaled());? Or is it okay to call WakeUp on an active thread? With the current impl, calling it while its awake would mean it would immediately wake up (consume the signal) the next time it tries to sleep. Also, please update the API to reflect the decision made here. Edit: just read comment on line 89. We should either disallow multiple WakeUps or make the API clear that they're allowed. First reading about it being a possibility in a comment at the end of ThreadMain is too late. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:66: static const size_t kDefaultStackSize = 0; No need for "static" on non-compound local const types (i.e. only for arrays/structs local consts). https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:74: // A SchedulerWorkerThread starts out sleeping. This should be mentioned in the API (header). https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:78: const RanTaskFromSequenceCallback& ran_task_from_sequence_callback, 1 callback is essentially a mini Delegate, 2 callbacks is still okay, but 3 Callbacks is when my review bell really starts thinking we should be merging them into a single Delegate here, especially if in practice all callbacks are all bound to dedicated methods on the caller's class. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:98: mutable SchedulerLock should_exit_for_testing_lock_; Instead of using a lock just for this here we could make |should_exit_for_testing_| volatile, also makes the impl cleaner IMO since it doesn't have to have locking scopes :-). Note: if I understand the C++ standard correctly I think this means you'll have to make the methods using |should_exit_for_testing_| volatile as well (same semantics as "const"). Never used volatile in C++ but curious whether it'd make things cleaner here :-)? https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:104: }; Where did the single threaded PQ go? https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: } Would be nice to test that the tests observed everything that was observable (i.e. all the counts were verified up to what they reached). As-is the test doesn't make sure the counts don't go past what is being observed and a regression doing twice the work could sneak in (I guess the EXPECT_EQ checks after the waits somewhat prevent that but still)? https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:200: TEST_F(TaskSchedulerWorkerThreadTest, ContinousWork) { Continuous Work ^ https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:212: const size_t expected_num_get_work_callback = kNumSequencesPerTest + 1; kExpectedNumGetWorkCallback https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:213: WaitForNumGetWorkCallback(expected_num_get_work_callback); If all sequences ran we shouldn't need to wait here. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:214: EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback()); if we don't need to wait above maybe make |num_get_work_callback_| volatile is sufficient and no need to use CV? https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:215: } Test num wakeups? Since it's stated in test description. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:237: } Test num wakeups?
Message was sent while issue was closed.
gab@: I addressed your comments about the unit tests in this CL: https://codereview.chromium.org/1863983006/ I'll create a separate CL for your other comments. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: } On 2016/04/05 23:35:20, gab wrote: > Would be nice to test that the tests observed everything that was observable > (i.e. all the counts were verified up to what they reached). As-is the test > doesn't make sure the counts don't go past what is being observed and a > regression doing twice the work could sneak in (I guess the EXPECT_EQ checks > after the waits somewhat prevent that but still)? Added "EXPECT_LE(num_run_tasks_, created_sequences_.size());" to RunTaskCallback() to make sure that we don't run more callbacks than returned by GetWork(). https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:200: TEST_F(TaskSchedulerWorkerThreadTest, ContinousWork) { On 2016/04/05 23:35:20, gab wrote: > Continuous Work > ^ Done. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:212: const size_t expected_num_get_work_callback = kNumSequencesPerTest + 1; On 2016/04/05 23:35:20, gab wrote: > kExpectedNumGetWorkCallback Done. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:213: WaitForNumGetWorkCallback(expected_num_get_work_callback); On 2016/04/05 23:35:20, gab wrote: > If all sequences ran we shouldn't need to wait here. Yes, we need to wait for the call to GetWork() that returns nullptr. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:214: EXPECT_EQ(expected_num_get_work_callback, NumGetWorkCallback()); On 2016/04/05 23:35:20, gab wrote: > if we don't need to wait above maybe make |num_get_work_callback_| volatile is > sufficient and no need to use CV? We do need to wait above. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:215: } On 2016/04/05 23:35:20, gab wrote: > Test num wakeups? Since it's stated in test description. I updated the test description. The test calls WakeUp() once... it's not something that we want to test. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:237: } On 2016/04/05 23:35:20, gab wrote: > Test num wakeups? ditto
Message was sent while issue was closed.
https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:38: wake_up_event_.Signal(); On 2016/04/05 23:35:20, gab wrote: > Should this DCHECK(!wake_up_event_.IsSignaled());? Or is it okay to call WakeUp > on an active thread? With the current impl, calling it while its awake would > mean it would immediately wake up (consume the signal) the next time it tries to > sleep. > > Also, please update the API to reflect the decision made here. > > Edit: just read comment on line 89. We should either disallow multiple WakeUps > or make the API clear that they're allowed. First reading about it being a > possibility in a comment at the end of ThreadMain is too late. We support calls to WakeUp on an active thread. It will be called every time that a Sequence is inserted into the single-threaded PQ. Updated the method's description. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:66: static const size_t kDefaultStackSize = 0; On 2016/04/05 23:35:20, gab wrote: > No need for "static" on non-compound local const types (i.e. only for > arrays/structs local consts). Done. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.cc:74: // A SchedulerWorkerThread starts out sleeping. On 2016/04/05 23:35:20, gab wrote: > This should be mentioned in the API (header). Done. https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:78: const RanTaskFromSequenceCallback& ran_task_from_sequence_callback, On 2016/04/05 23:35:20, gab wrote: > 1 callback is essentially a mini Delegate, 2 callbacks is still okay, but 3 > Callbacks is when my review bell really starts thinking we should be merging > them into a single Delegate here, especially if in practice all callbacks are > all bound to dedicated methods on the caller's class. Done here https://codereview.chromium.org/1864333002/ https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:98: mutable SchedulerLock should_exit_for_testing_lock_; On 2016/04/05 23:35:20, gab wrote: > Instead of using a lock just for this here we could make > |should_exit_for_testing_| volatile, also makes the impl cleaner IMO since it > doesn't have to have locking scopes :-). > > Note: if I understand the C++ standard correctly I think this means you'll have > to make the methods using |should_exit_for_testing_| volatile as well (same > semantics as "const"). Never used volatile in C++ but curious whether it'd make > things cleaner here :-)? If should_exit_for_testing_ is volatile: Thread 1: should_exit_for_testing_ = true; wake_up_event_.Signal(); Thread 2: wake_up_event_.Wait(); if (should_exit_for_testing_) {} There is no guarantee that the condition is true on thread 2 unless we know that WaitableEvent::Signal/Wait both have visible side effects. I'm pretty sure that these methods have visible side effects (pending input from etienneb@), but I don't like relying on that fact. "within a single thread of execution, volatile accesses cannot be optimized out or reordered with another visible side effect that is sequenced-before or sequenced-after the volatile access. This makes volatile objects suitable for communication with a signal handler, but not with another thread of execution, see std::memory_order" http://en.cppreference.com/w/cpp/language/cv https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:104: }; On 2016/04/05 23:35:20, gab wrote: > Where did the single threaded PQ go? I will add it in another CL. It will be probably be owned by SchedulerWorkerThread, but could also be owned by SchedulerThreadPool.
Message was sent while issue was closed.
Thanks for the follow-up CLs, responding to other discussion below, thanks! https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1704113002/diff/320002/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:98: mutable SchedulerLock should_exit_for_testing_lock_; On 2016/04/07 16:07:43, fdoray wrote: > On 2016/04/05 23:35:20, gab wrote: > > Instead of using a lock just for this here we could make > > |should_exit_for_testing_| volatile, also makes the impl cleaner IMO since it > > doesn't have to have locking scopes :-). > > > > Note: if I understand the C++ standard correctly I think this means you'll > have > > to make the methods using |should_exit_for_testing_| volatile as well (same > > semantics as "const"). Never used volatile in C++ but curious whether it'd > make > > things cleaner here :-)? > > If should_exit_for_testing_ is volatile: > > Thread 1: > should_exit_for_testing_ = true; > wake_up_event_.Signal(); > > Thread 2: > wake_up_event_.Wait(); > if (should_exit_for_testing_) {} > > There is no guarantee that the condition is true on thread 2 unless we know that > WaitableEvent::Signal/Wait both have visible side effects. I'm pretty sure that > these methods have visible side effects (pending input from etienneb@), but I > don't like relying on that fact. > > "within a single thread of execution, volatile accesses cannot be optimized out > or reordered with another visible side effect that is sequenced-before or > sequenced-after the volatile access. This makes volatile objects suitable for > communication with a signal handler, but not with another thread of execution, > see std::memory_order" http://en.cppreference.com/w/cpp/language/cv > Ah interesting, so it's not like Java's volatile which, IIRC, is essentially like wrapping all access to |foo| into a |synchronized(foo){}| block. I guess that's because Java natively supports multi-threading whereas C++ doesn't. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
