Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
439923006:
Prioritizing input and compositor tasks in the blink scheduler. (Closed)
|
Created:
6 years, 4 months ago by alex clarke (OOO till 29th) Modified:
6 years, 4 months ago CC:
abarth-chromium, blink-reviews, blink-reviews-wtf_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionPrioritizing input and compositor tasks in the blink scheduler.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180171
Patch Set 1 #
Total comments: 20
Patch Set 2 : Responding to feedback, and removing atomics #Patch Set 3 : Added a couple of tests. #Patch Set 4 : Un deleted something I didn't intend to delete #
Total comments: 15
Patch Set 5 : changed runPendingTasks to only run low prioritty task at a time #Patch Set 6 : Responding to more feedback #Patch Set 7 : made runPendingTasks private #
Total comments: 17
Patch Set 8 : Respoding to more feedback #
Total comments: 14
Patch Set 9 : Responding to Eric's comments. #Patch Set 10 : change to pacify clang #
Total comments: 27
Patch Set 11 : Responding to Sami's comments #
Total comments: 28
Patch Set 12 : Rebase + respond to Eric's feedback #
Total comments: 2
Patch Set 13 : moving DoubleBufferedDeque #Patch Set 14 : Removing unwanted change #Patch Set 15 : Adding wtf/DoubleBufferedDeque.h to git #
Total comments: 22
Patch Set 16 : Responding to the latest round of comments. #Patch Set 17 : Fixed a couple of issues. #Patch Set 18 : Removing a couple of tests that fail due to the assert we added. #
Total comments: 16
Patch Set 19 : Responded to Sami's comments #Patch Set 20 : Removed a couple of changes I didn't intend to be in this patch. #
Total comments: 4
Patch Set 21 : Responding to Sami's feedback #
Total comments: 10
Patch Set 22 : Responding to Eric's feedback #
Total comments: 4
Patch Set 23 : Small chanegs for Sami #Patch Set 24 : Couple of tweaks for Sami #Patch Set 25 : Adding a missing #include #
Messages
Total messages: 48 (0 generated)
Thanks Alex, I think this looks great. I left some nit picks, but mainly I think we should ensure low priority tasks aren't able to completely own the main thread. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... File Source/platform/scheduler/AtomicRingbuffer.h (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:17: AtomicRingBuffer() : m_writeCounter(0), m_readIndex(1) nit: Blink has a different style for initializers, see http://www.chromium.org/blink/coding-style https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:28: while (!atomicTestAndSwap((void* volatile*) &m_values[pos], 0, value)) { nit: nullptr instead of 0? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:36: T* value = (T*) atomicExchange((void* volatile*) &m_values[m_readIndex], nullptr); Please use C++-style casts instead of C ones. Is the "*" after the void redundant? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:43: volatile int m_writeCounter; nit: m_writeIndex to match the other var? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:133: TracedTask* task = m_pendingInputTasks.pop(); How about making this slightly safer by wrapping the pointer into a scoped container: OwnPtr<TracedTask> task = adoptPtr(m_pendingInputTasks.pop()); https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:150: m_pendingBlinkTasks.takeFirst().run(); It seems like it's possible for low priority tasks to completely hog the main thread if new ones just keep getting posted. How about just running one low priority task per call to runPendingTasks() and if more are pending just call maybePostTaskLoopOnMainThread()? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:75: TracedTask(Task task, TraceLocation location) : m_task(task), m_location(location) { } nit: Initializer style. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:85: AtomicRingBuffer<TracedTask, 128> m_pendingInputTasks; One pattern we could steal from cc/ is to put members into different structs depending on which thread they are accessed from. For example, there's a MainThreadOnly struct and a main() function that returns a reference to it and also asserts the caller is on the main thread. Maybe here we could have a struct for the shared state and everything else would be main thread only. WDYT? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:89: WTF::Deque<TracedTask> m_pendingBlinkTasks; m_pendingLowPriorityTasks or something? https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:94: void setBlinkShouldYield(); How about setHighPriorityWorkPending to match shouldYieldForHighPriorityWork?
https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... File Source/platform/scheduler/AtomicRingbuffer.h (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:17: AtomicRingBuffer() : m_writeCounter(0), m_readIndex(1) On 2014/08/06 10:19:52, Sami wrote: > nit: Blink has a different style for initializers, see > http://www.chromium.org/blink/coding-style Acknowledged. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:28: while (!atomicTestAndSwap((void* volatile*) &m_values[pos], 0, value)) { On 2014/08/06 10:19:52, Sami wrote: > nit: nullptr instead of 0? Acknowledged. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:36: T* value = (T*) atomicExchange((void* volatile*) &m_values[m_readIndex], nullptr); On 2014/08/06 10:19:52, Sami wrote: > Please use C++-style casts instead of C ones. Is the "*" after the void > redundant? Acknowledged. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/At... Source/platform/scheduler/AtomicRingbuffer.h:43: volatile int m_writeCounter; On 2014/08/06 10:19:52, Sami wrote: > nit: m_writeIndex to match the other var? Acknowledged. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:133: TracedTask* task = m_pendingInputTasks.pop(); On 2014/08/06 10:19:52, Sami wrote: > How about making this slightly safer by wrapping the pointer into a scoped > container: > > OwnPtr<TracedTask> task = adoptPtr(m_pendingInputTasks.pop()); Done. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:150: m_pendingBlinkTasks.takeFirst().run(); On 2014/08/06 10:19:52, Sami wrote: > It seems like it's possible for low priority tasks to completely hog the main > thread if new ones just keep getting posted. How about just running one low > priority task per call to runPendingTasks() and if more are pending just call > maybePostTaskLoopOnMainThread()? It's also possible for compositor tasks to do something similar. E.g. If we're on a very heavy page, it's possible for multiple calls to BeginMainFrame to buffer up. Lets discuss this offline. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:75: TracedTask(Task task, TraceLocation location) : m_task(task), m_location(location) { } On 2014/08/06 10:19:52, Sami wrote: > nit: Initializer style. Done. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:85: AtomicRingBuffer<TracedTask, 128> m_pendingInputTasks; On 2014/08/06 10:19:52, Sami wrote: > One pattern we could steal from cc/ is to put members into different structs > depending on which thread they are accessed from. For example, there's a > MainThreadOnly struct and a main() function that returns a reference to it and > also asserts the caller is on the main thread. Maybe here we could have a struct > for the shared state and everything else would be main thread only. WDYT? I quite like that pattern, however I don't think we need it yet. I've found a way to eliminate the need for the WTF::Deque - if one of the ring buffers becomes full, we'll fall back to using the MainThreadTaskAdapter to process tasks, so it's now safe for m_pendingLowPriorityTasks to be an AtomicRingBuffer since it no longer spinlocks when full. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:89: WTF::Deque<TracedTask> m_pendingBlinkTasks; On 2014/08/06 10:19:52, Sami wrote: > m_pendingLowPriorityTasks or something? Done. https://codereview.chromium.org/439923006/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:94: void setBlinkShouldYield(); On 2014/08/06 10:19:52, Sami wrote: > How about setHighPriorityWorkPending to match shouldYieldForHighPriorityWork? I've decided not to use atomics, nailing down all the corner cases was getting too complicated.
https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:64: WeakPtr<MainThreadPendingTaskRunner> MainThreadPendingTaskRunner::s_taskInFlight; I think we need to avoid non-POD static initializers. There's a DEFINE_GLOBAL macro if we really need this, but is there a way we could avoid it by maybe keeping track of whether the task is pending with a normal (atomic) bool? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:56: void runPendingTasks(); This one doesn't need to be public, does it? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:71: TracedTask(Task task, TraceLocation location) const Task& and const TraceLocation& https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:90: bool isEmpty() Could we somehow combine isEmpty() and takeFirst() so that we only take the lock once? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:112: void (*m_sharedTimerFunction)(); This member should only be used on the main thread, right? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:114: void maybePostTaskLoopOnMainThread(); Could you move these two before the data member declarations? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:137: void appendToVector(String value) nit: looks like this could be a normal std::string? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:154: std::vector<int> m_intOrder; bikeshed: m_reentrantOrder? https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:252: Would you mind adding a test that checks all pending tasks are executed when the scheduler goes away?
https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:64: WeakPtr<MainThreadPendingTaskRunner> MainThreadPendingTaskRunner::s_taskInFlight; On 2014/08/07 14:24:45, Sami wrote: > I think we need to avoid non-POD static initializers. There's a DEFINE_GLOBAL > macro if we really need this, but is there a way we could avoid it by maybe > keeping track of whether the task is pending with a normal (atomic) bool? Done. https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:71: TracedTask(Task task, TraceLocation location) On 2014/08/07 14:24:45, Sami wrote: > const Task& and const TraceLocation& Done. https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:90: bool isEmpty() On 2014/08/07 14:24:45, Sami wrote: > Could we somehow combine isEmpty() and takeFirst() so that we only take the lock > once? Done. https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:137: void appendToVector(String value) On 2014/08/07 14:24:45, Sami wrote: > nit: looks like this could be a normal std::string? Done. https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:154: std::vector<int> m_intOrder; On 2014/08/07 14:24:45, Sami wrote: > bikeshed: m_reentrantOrder? Done. https://codereview.chromium.org/439923006/diff/60001/Source/platform/schedule... Source/platform/scheduler/SchedulerTest.cpp:252: On 2014/08/07 14:24:45, Sami wrote: > Would you mind adding a test that checks all pending tasks are executed when the > scheduler goes away? Done.
Nice, getting close :) https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:57: Scheduler::shared()->runPendingTasks(); It would be slightly neater to pass in the scheduler instance to this class instead of relying on a global. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:84: , m_mainThreadInFlight(0) Bikeshedding again: m_mainThreadTaskRunnerCount? https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:145: m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight)); Why not just call maybePostTaskLoopOnMainThread here? https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:161: void Scheduler::maybePostTaskLoopOnMainThread() My bikeshed sense is tingling again :) I like having a single name for a single thing, so how about maybePostMainThreadPendingTaskRunner()? https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:166: m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight)); It's worth noting that this can cause several task runners to get posted. I don't think that should cause any real problems though. Let's still add a comment here. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:22: class TraceLocation; I think this forward declaration can be removed now. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:93: class MainThreadPendingTaskRunner; It doesn't look like this declaration is needed? https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:100: void (*m_sharedTimerFunction)(); This should be separated from the cross-thread fields. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:103: void maybePostTaskLoopOnMainThread(); Looks like these got moved after the data members again. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:107: friend class MainThreadPendingTaskRunner; Could you move this right after the private: line? https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:202: Scheduler::shutdown(); Thanks for adding this test. Can you add a re-entrant one here too since those are the tricky ones?
https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:57: Scheduler::shared()->runPendingTasks(); On 2014/08/07 15:51:26, Sami wrote: > It would be slightly neater to pass in the scheduler instance to this class > instead of relying on a global. Done. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:84: , m_mainThreadInFlight(0) On 2014/08/07 15:51:26, Sami wrote: > Bikeshedding again: m_mainThreadTaskRunnerCount? Done. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:145: m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight)); On 2014/08/07 15:51:26, Sami wrote: > Why not just call maybePostTaskLoopOnMainThread here? Because the reference count is not zero. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:161: void Scheduler::maybePostTaskLoopOnMainThread() On 2014/08/07 15:51:26, Sami wrote: > My bikeshed sense is tingling again :) I like having a single name for a single > thing, so how about maybePostMainThreadPendingTaskRunner()? Done. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:166: m_mainThread->postTask(new MainThreadPendingTaskRunner(&m_mainThreadInFlight)); On 2014/08/07 15:51:26, Sami wrote: > It's worth noting that this can cause several task runners to get posted. I > don't think that should cause any real problems though. Let's still add a > comment here. Done. https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/120001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:202: Scheduler::shutdown(); On 2014/08/07 15:51:27, Sami wrote: > Thanks for adding this test. Can you add a re-entrant one here too since those > are the tricky ones? Done.
If this is only a main-task scheduler, why do we need any locks at all? What other threads are going to use this? https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) Thank you for including the type in the name. :) I still wish we had a time type for Blink so that the compiler could help us distinguish between tehm. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:67: class TracedTask { Why aren't all tasks's traced? I think we want one task type in all of Blink that just has all teh bells and whistles. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:97: LockingTracedTaskDeque m_pendingInputTasks; Do these want separate locks or one big global lock? This feels like we're re-inventing MessageQueue from chromium? Chromium avoids locks on each trip through the message loop by not locking on the main queue and only locking when swapping the pending and active queues. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:101: volatile int m_mainThreadTaskRunnerCount; volatile? I've never seen this used in Blink. I presume this makes compilers not be able to cache the value on the cpu, etc?
https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) On 2014/08/07 17:18:15, eseidel wrote: > Thank you for including the type in the name. :) I still wish we had a time > type for Blink so that the compiler could help us distinguish between tehm. +1, I wouldn't object to a clone of base/time.h's TimeTicks and TimeDelta, because besides units you very often also need to deal with durations. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:67: class TracedTask { On 2014/08/07 17:18:15, eseidel wrote: > Why aren't all tasks's traced? I think we want one task type in all of Blink > that just has all teh bells and whistles. This is just an internal helper class, but I guess we could promote it to an actual class to scheduler/Task.h (replacing the typedef here) and make sure it always has an associated TraceLocation. That would also fix the bug that idle tasks currently have no TraceLocation. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:97: LockingTracedTaskDeque m_pendingInputTasks; On 2014/08/07 17:18:15, eseidel wrote: > Do these want separate locks or one big global lock? This feels like we're > re-inventing MessageQueue from chromium? I guess a global lock would be better if there is little contention, which I believe is the case here, because input and compositing tasks are generated during different parts of the vsync cycle. > Chromium avoids locks on each trip through the message loop by not locking on > the main queue and only locking when swapping the pending and active queues. I like the double buffered queue idea.
https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:101: volatile int m_mainThreadTaskRunnerCount; On 2014/08/07 17:18:15, eseidel wrote: > volatile? I've never seen this used in Blink. I presume this makes compilers > not be able to cache the value on the cpu, etc? Yes, it's needed for atomic operations (and also used by wtf/Atomics.h). Again base has a typedef to hide this ugliness which I think we should adopt.
Sorry for missing your top-level question: On 2014/08/07 17:18:15, eseidel wrote: > If this is only a main-task scheduler, why do we need any locks at all? What > other threads are going to use this? Input tasks are generated on the renderer's IO thread, compositor tasks come from the cc thread and Scheduler::postTask() may be called by the background parsing and other worker threads as WTF::callOnMain() hooks into it. In other words all of these tasks want to run on the main thread, but they are still scheduled from other threads.
But we could keep the scheduler lock-less and just have callOnMainThread handle the locking, no?
On 2014/08/07 21:47:04, eseidel wrote:
> But we could keep the scheduler lock-less and just have callOnMainThread
handle
> the locking, no?
I'm not sure how that would work: callOnMainThread is a producer of tasks and
the scheduler is a consumer and the task queue is the shared resource between
them that needs locking. There's a similar relationship for input and compositor
tasks.
To guarantee confusion, here's some ascii art showing which threads are feeding
tasks into the scheduler concurrently:
renderer IO ---[input]
\
cc -----------\-----[beginFrame]
\ \
parser -------------\----------------\-----[WTF::callOnMain]
\ \ \
blink [postInputTask] [postCompositorTask] [postTask]
main
To make this possible we need *some* locking, and it seems to me the scheduler
is the right place for it because that's where the shared resource, i.e., the
task queue, lives.
https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) On 2014/08/07 17:18:15, eseidel wrote: > Thank you for including the type in the name. :) I still wish we had a time > type for Blink so that the compiler could help us distinguish between tehm. Acknowledged. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) On 2014/08/07 18:22:21, Sami wrote: > On 2014/08/07 17:18:15, eseidel wrote: > > Thank you for including the type in the name. :) I still wish we had a time > > type for Blink so that the compiler could help us distinguish between tehm. > > +1, I wouldn't object to a clone of base/time.h's TimeTicks and TimeDelta, > because besides units you very often also need to deal with durations. I also think that's a good idea, but lets do it in a follow up cl? https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:67: class TracedTask { On 2014/08/07 17:18:15, eseidel wrote: > Why aren't all tasks's traced? I think we want one task type in all of Blink > that just has all teh bells and whistles. I'm not sure if you're suggesting we make TracedTask public or not. We could make it public, but it would mean more typing for the clients of this class, and the postTask methods would have a different looking interface than the chromium postTask. What do you think? https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:97: LockingTracedTaskDeque m_pendingInputTasks; On 2014/08/07 17:18:15, eseidel wrote: > Do these want separate locks or one big global lock? This feels like we're > re-inventing MessageQueue from chromium? > > Chromium avoids locks on each trip through the message loop by not locking on > the main queue and only locking when swapping the pending and active queues. Double buffering the queues is an interesting idea. I'm quite keen to avoid locks if I could but to make it properly thread safe, the incoming queue needs locks on append and for the swap. I notice that's what chromium's IncomingTaskQueue does. The advantage to double buffering is pulling tasks off the non-incoming queue no longer needs locks. So I think I'll try that, although ideally it would be nice to use a lockfree deque. Boost has one but I don't know if it works on all platforms. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:101: volatile int m_mainThreadTaskRunnerCount; On 2014/08/07 17:18:15, eseidel wrote: > volatile? I've never seen this used in Blink. I presume this makes compilers > not be able to cache the value on the cpu, etc? Pretty much. It's used in a few places in blink: https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&typ... It's worth noting that volatile by itself does not imply thread safety, you need atomics, memory barriers or locks in addition.
I think this is starting to converge. https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:67: class TracedTask { On 2014/08/08 13:32:23, alexclarke wrote: > On 2014/08/07 17:18:15, eseidel wrote: > > Why aren't all tasks's traced? I think we want one task type in all of Blink > > that just has all teh bells and whistles. > > I'm not sure if you're suggesting we make TracedTask public or not. > > We could make it public, but it would mean more typing for the clients of this > class, and the postTask methods would have a different looking interface than > the chromium postTask. > > What do you think? I think I agree that matching base's PostTask() interface is probably the better choice and results in much less typing at the call site. We should fix idle tasks though (in a different CL). https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDeque.h:1: #ifndef DoubleBufferedDeque_h Please add a copyright header. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDeque.h:30: Locker<Mutex> lock(m_mutex); Let's DCHECK that the new queue is empty here. If there are any tasks left in there, we'll get an ordering bug. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDequeTest.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDequeTest.cpp:39: DoubleBufferedDeque<int> queue; It's a minor thing but could you rewrite this test so that it doesn't hang on to the old buffer after swapBuffers? Doing that in production code would be bad. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:43: explicit MainThreadPendingHighPriorityTaskRunner() No need for |explicit| since there are no parameters. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:46: if (!scheduler) This condition should never fail since the scheduler is the one creating this class, right? https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:63: if (!scheduler) Do we really need these safe guards? I guess these could get posted right before shutdown so I guess we do... https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:71: explicit MainThreadPendingTaskRunner( No need for |explicit| since there are two parameters. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:77: if (!scheduler) Ditto about scheduler always being there. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:135: runHighPriorityTasks(); Why two calls? https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:227: return !m_pendingInputTasks.isEmpty() || !m_pendingCompositorTasks.isEmpty(); If we're worried about the polling here, we could add another atomic (m_pendingHighPriorityTaskCount) that we check here. This is probably good enough for now though. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:86: void maybePostMainThreadPendingHighPriorityTaskRunner(); Can you move these up before the data members? https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:89: class MainThreadPendingTaskRunner; These should also go right after the private: label. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:150: // that the Scheduler still exists befpre making the re-entrant call. s/befpre/before/ https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:325: nit: Some extra whitespace here.
This CL really looks like a re-implementation of base::MessageLoop. Should we write the code in //base instead where we don't need to re-invent the base::MessageLoop wheel?
https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDeque.h:30: Locker<Mutex> lock(m_mutex); On 2014/08/08 15:31:13, Sami wrote: > Let's DCHECK that the new queue is empty here. If there are any tasks left in > there, we'll get an ordering bug. Good idea, although I'll need to change one of the tests slightly because of this :) https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDequeTest.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDequeTest.cpp:39: DoubleBufferedDeque<int> queue; On 2014/08/08 15:31:13, Sami wrote: > It's a minor thing but could you rewrite this test so that it doesn't hang on to > the old buffer after swapBuffers? Doing that in production code would be bad. Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:43: explicit MainThreadPendingHighPriorityTaskRunner() On 2014/08/08 15:31:13, Sami wrote: > No need for |explicit| since there are no parameters. Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:46: if (!scheduler) On 2014/08/08 15:31:13, Sami wrote: > This condition should never fail since the scheduler is the one creating this > class, right? I'll dcheck it instead https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:63: if (!scheduler) On 2014/08/08 15:31:13, Sami wrote: > Do we really need these safe guards? I guess these could get posted right before > shutdown so I guess we do... Unfortunately yes for that reason :( https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:71: explicit MainThreadPendingTaskRunner( On 2014/08/08 15:31:13, Sami wrote: > No need for |explicit| since there are two parameters. Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:77: if (!scheduler) On 2014/08/08 15:31:13, Sami wrote: > Ditto about scheduler always being there. Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:135: runHighPriorityTasks(); On 2014/08/08 15:31:13, Sami wrote: > Why two calls? Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:227: return !m_pendingInputTasks.isEmpty() || !m_pendingCompositorTasks.isEmpty(); On 2014/08/08 15:31:13, Sami wrote: > If we're worried about the polling here, we could add another atomic > (m_pendingHighPriorityTaskCount) that we check here. This is probably good > enough for now though. Lets see if this pops up on the profiles. If it does we can do that. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:86: void maybePostMainThreadPendingHighPriorityTaskRunner(); On 2014/08/08 15:31:13, Sami wrote: > Can you move these up before the data members? Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:89: class MainThreadPendingTaskRunner; On 2014/08/08 15:31:13, Sami wrote: > These should also go right after the private: label. Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:150: // that the Scheduler still exists befpre making the re-entrant call. On 2014/08/08 15:31:13, Sami wrote: > s/befpre/before/ Done. https://codereview.chromium.org/439923006/diff/180001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:325: On 2014/08/08 15:31:13, Sami wrote: > nit: Some extra whitespace here. Done.
On 2014/08/08 15:43:13, abarth wrote: > This CL really looks like a re-implementation of base::MessageLoop. Should we > write the code in //base instead where we don't need to re-invent the > base::MessageLoop wheel? We also had this thought during the design process, but ended up thinking this belongs in Blink instead. This is mainly because the knowledge we want to use for making scheduling decisions doesn't feel like it belongs in base. With that I mean things like coordinating idle tasks with the frame deadline, avoiding low priority tasks if the user is touching the screen and overall being able to prioritize latency-critical tasks over others. Maybe base::MessageLoop could be extended to delegate some of its scheduling decisions here, but my feeling is that would only result in lots of 3-sided patch churn with no real benefit for any other MessageLoop client except us. WDYT?
https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:35: Scheduler::IdleTask m_idleTask; Why are idle tasks different from tasks? https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:36: double m_allottedTimeMs; Filed cbug.com/402027 about having real types for these. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:41: class Scheduler::MainThreadPendingHighPriorityTaskRunner : public blink::WebThread::Task { Don't need blink:: https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:46: atomicIncrement(&Scheduler::shared()->m_mainThreadTaskRunnerCount); Why are we grabbing at a m_? Instead of using a function? https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:147: m_pendingInputTasks.append(TracedTask(task, location)); I really think we want all tasks to be traced. Tracing gives us a huge debuggability benifit. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:168: runHighPriorityTasks(); Why twice? If the sharedTimer function has chosen to yield due to spending too much time are we sure we want to run these now? https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:175: WTF::Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); WTF:: should not be needed. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:56: class MainThreadPendingTaskRunner; Do you need these forward declarations in order to use the class? Also, doesn't this forward-declare them as classes scoped to this class's namespace? That seems wrong. I would expect you'd wnat these at the top inside blink { but not inside the class itself. I worry some compiler variant might choke. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:71: class TracedTask { How is this different from Task? Can all Tasks's be traced? Also, shouldnt' this be Noncopyable? https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:88: blink::WebThread* m_mainThread; You shouldn't need blink:: here. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:152: Scheduler::shared()->postTask( No need to wrap to 80c in Blink. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:161: if (m_reentrantCount <= 4) { I would have written these as early returns. If (reeentrantCount > 4) return; https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:180: std::vector<string> m_order; Is there a reason to use std:: types instead of wtf types? They have different symantics and both are valid, but most of blink:: uses wtf types. I guess less so for gtests... https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:308: m_scheduler->postInputTask( No need to wrap to 80c like chromium.
There was a different approach to teh scheduilng wheel which did extend base instead: https://docs.google.com/document/d/1SWpjgtwIaL_hIcbm6uGJKZ8o8R9xYre-yG0VDOjFB... https://codereview.chromium.org/281073002/
Ok. Thanks for considering that option.
Lets walk through this at/after our meeting today so we can finally get this landed. Sorry this has taken so long. https://codereview.chromium.org/439923006/diff/220001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/220001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDeque.h:13: template <typename T> class DoubleBufferedDeque { Base datatypes like this probably belong in wtf.
Notes while we were talking. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (left): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: { If this can't be created from any thread, we should RELEASE_ASSERT(isMainThread()); https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:19: class MainThreadIdleTaskAdapter : public WebThread::Task { Might add a comment to note that this can be created from any thread. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) Might have called this timeout or maxDuration, but this is also fine. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:64: Scheduler* scheduler = Scheduler::shared(); When can this happen? This feels like it should be a RELEASE_ASSERT(scheduler); https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:85: if (!scheduler) Feels like we should flush the tasks before shut-down so as not to need this. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:93: TRACE_EVENT2("blink", "MainThreadPendingTaskRunner::run", Currious there is a trace here but not the other? https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:99: m_task(); Why would we want to run this if !scheduler? https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:189: Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); This is kinda neat. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:249: return acquireLoad(&m_highPriotityTaskCount) != 0; Is it concerning that this is being called often? https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:87: void incrementMainThreadTaskRunnerCount(); Please add a comment to explain what these are for. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:88: void decrementMainThreadTaskRunnerCount(); we should also add a comment that the only reason thse are not a bool is due to our limited WTF atomics.
https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:35: Scheduler::IdleTask m_idleTask; On 2014/08/08 17:31:06, eseidel wrote: > Why are idle tasks different from tasks? Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:36: double m_allottedTimeMs; On 2014/08/08 17:31:06, eseidel wrote: > Filed cbug.com/402027 about having real types for these. Acknowledged. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:41: class Scheduler::MainThreadPendingHighPriorityTaskRunner : public blink::WebThread::Task { On 2014/08/08 17:31:06, eseidel wrote: > Don't need blink:: Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:46: atomicIncrement(&Scheduler::shared()->m_mainThreadTaskRunnerCount); On 2014/08/08 17:31:06, eseidel wrote: > Why are we grabbing at a m_? Instead of using a function? Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:147: m_pendingInputTasks.append(TracedTask(task, location)); On 2014/08/08 17:31:06, eseidel wrote: > I really think we want all tasks to be traced. Tracing gives us a huge > debuggability benifit. Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:168: runHighPriorityTasks(); On 2014/08/08 17:31:06, eseidel wrote: > Why twice? If the sharedTimer function has chosen to yield due to spending too > much time are we sure we want to run these now? I've added some comments to explain this. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:175: WTF::Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); On 2014/08/08 17:31:06, eseidel wrote: > WTF:: should not be needed. Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:56: class MainThreadPendingTaskRunner; On 2014/08/08 17:31:07, eseidel wrote: > Do you need these forward declarations in order to use the class? gcc requires this sadly. > Also, doesn't > this forward-declare them as classes scoped to this class's namespace? That > seems wrong. We don't want incrementMainThreadTaskRunnerCount(), decrementMainThreadTaskRunnerCount() or runHighPriorityTasks() to be part of the public interface. If this was java those methods would be package protected. > I would expect you'd wnat these at the top inside blink { but not > inside the class itself. I worry some compiler variant might choke. I'd be surprised if we where going to have problems here since Lakos style PIMPLs (http://advancedcpp.livejournal.com/9332.html) have been commonly used in C++ for many years. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:71: class TracedTask { On 2014/08/08 17:31:07, eseidel wrote: > How is this different from Task? Can all Tasks's be traced? Done. > Also, shouldnt' this be Noncopyable? No since these are held inside a WTF::Deque which does copying. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:88: blink::WebThread* m_mainThread; On 2014/08/08 17:31:07, eseidel wrote: > You shouldn't need blink:: here. Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:152: Scheduler::shared()->postTask( On 2014/08/08 17:31:07, eseidel wrote: > No need to wrap to 80c in Blink. Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:161: if (m_reentrantCount <= 4) { On 2014/08/08 17:31:07, eseidel wrote: > I would have written these as early returns. > > If (reeentrantCount > 4) return; Done. https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:180: std::vector<string> m_order; On 2014/08/08 17:31:07, eseidel wrote: > Is there a reason to use std:: types instead of wtf types? They have different > symantics and both are valid, but most of blink:: uses wtf types. I guess less > so for gtests... EXPECT_THAT(...) doesn't work with WTF::Vector https://codereview.chromium.org/439923006/diff/200001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:308: m_scheduler->postInputTask( On 2014/08/08 17:31:07, eseidel wrote: > No need to wrap to 80c like chromium. Done. https://codereview.chromium.org/439923006/diff/220001/Source/platform/schedul... File Source/platform/scheduler/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/220001/Source/platform/schedul... Source/platform/scheduler/DoubleBufferedDeque.h:13: template <typename T> class DoubleBufferedDeque { On 2014/08/11 15:19:14, eseidel wrote: > Base datatypes like this probably belong in wtf. Done. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (left): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: { On 2014/08/11 17:02:36, eseidel wrote: > If this can't be created from any thread, we should > RELEASE_ASSERT(isMainThread()); It can be called from any thread. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:19: class MainThreadIdleTaskAdapter : public WebThread::Task { On 2014/08/11 17:02:36, eseidel wrote: > Might add a comment to note that this can be created from any thread. Done. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:23: , m_allottedTimeMs(allottedTimeMs) On 2014/08/11 17:02:36, eseidel wrote: > Might have called this timeout or maxDuration, but this is also fine. Acknowledged. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:64: Scheduler* scheduler = Scheduler::shared(); On 2014/08/11 17:02:36, eseidel wrote: > When can this happen? This feels like it should be a RELEASE_ASSERT(scheduler); Because a posted MainThreadPendingHighPriorityTaskRunner can outlive the scheduler. Since we (probably) can't draining tasks posted to the run loop in the scheduler's destructor, there's no avoiding this. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:85: if (!scheduler) On 2014/08/11 17:02:36, eseidel wrote: > Feels like we should flush the tasks before shut-down so as not to need this. We flush what we can (high priority tasks) but since this runs on the main task loop, its lifetime can exceed the Scheduler. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:93: TRACE_EVENT2("blink", "MainThreadPendingTaskRunner::run", On 2014/08/11 17:02:36, eseidel wrote: > Currious there is a trace here but not the other? Because Scheduler::TracedTask::run() does that for us. Perhaps this class should contain a TracedTask for consistency. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:99: m_task(); On 2014/08/11 17:02:36, eseidel wrote: > Why would we want to run this if !scheduler? Because this is the existing behaviour but we've added an assert to try and catch this situation. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:189: Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); On 2014/08/11 17:02:36, eseidel wrote: > This is kinda neat. Acknowledged. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:249: return acquireLoad(&m_highPriotityTaskCount) != 0; On 2014/08/11 17:02:36, eseidel wrote: > Is it concerning that this is being called often? Ultimately if we refactor all the blink timers we can do away with this, but that's a lot of work. (I'm not currently sure if it's worth doing) A middle ground might be to get the thread timers to post tasks via the scheduler under the hood, which would also let us do away with this. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:87: void incrementMainThreadTaskRunnerCount(); On 2014/08/11 17:02:36, eseidel wrote: > Please add a comment to explain what these are for. They've gone. https://codereview.chromium.org/439923006/diff/280001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:88: void decrementMainThreadTaskRunnerCount(); On 2014/08/11 17:02:36, eseidel wrote: > we should also add a comment that the only reason thse are not a bool is due to > our limited WTF atomics. They've gone.
https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:58: Scheduler* scheduler = Scheduler::shared(); Looks like the destructor should be empty here? https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:87: Scheduler* scheduler = Scheduler::shared(); Same here? https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:133: while (shouldYieldForHighPriorityWork()) { I'm slightly worried that shouldYieldForHighPriorityWork will end up having more complex logic and will end up breaking this function. Let's add an internal hasPendingHighPriorityWork() helper instead and have shouldYieldForHighPriorityWork() call that. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:195: atomicDecrement(&m_highPriotityTaskCount); I think we can replace these atomics with just one atomicSubtract outside the loop because the task lists won't get modified during execution anymore. I think we could also rewrite this to be a little simpler: while (!inputTasks.isEmpty()) inputTasks.takeFirst.run(); while (!compositorTasks.isEmpty()) compositorTasks.takeFirst.run(); https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:21: class TraceLocation; Could you remove this forward decl now that we're including the header? https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:84: void maybePostMainThreadPendingHighPriorityTaskRunner(); Not needed any longer, right? https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:89: void (*m_sharedTimerFunction)(); I think this is main thread only. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:96: volatile int m_highPriotityTaskCount; Typo: Priority
https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:58: Scheduler* scheduler = Scheduler::shared(); On 2014/08/12 13:02:59, Sami wrote: > Looks like the destructor should be empty here? Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:87: Scheduler* scheduler = Scheduler::shared(); On 2014/08/12 13:02:59, Sami wrote: > Same here? Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:133: while (shouldYieldForHighPriorityWork()) { On 2014/08/12 13:02:59, Sami wrote: > I'm slightly worried that shouldYieldForHighPriorityWork will end up having more > complex logic and will end up breaking this function. Let's add an internal > hasPendingHighPriorityWork() helper instead and have > shouldYieldForHighPriorityWork() call that. Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:195: atomicDecrement(&m_highPriotityTaskCount); On 2014/08/12 13:02:59, Sami wrote: > I think we can replace these atomics with just one atomicSubtract outside the > loop because the task lists won't get modified during execution anymore. > > I think we could also rewrite this to be a little simpler: > > while (!inputTasks.isEmpty()) > inputTasks.takeFirst.run(); > > while (!compositorTasks.isEmpty()) > compositorTasks.takeFirst.run(); Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:21: class TraceLocation; On 2014/08/12 13:02:59, Sami wrote: > Could you remove this forward decl now that we're including the header? Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:84: void maybePostMainThreadPendingHighPriorityTaskRunner(); On 2014/08/12 13:02:59, Sami wrote: > Not needed any longer, right? Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:89: void (*m_sharedTimerFunction)(); On 2014/08/12 13:02:59, Sami wrote: > I think this is main thread only. Done. https://codereview.chromium.org/439923006/diff/310007/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:96: volatile int m_highPriotityTaskCount; On 2014/08/12 13:02:59, Sami wrote: > Typo: Priority Done.
Thanks Alex, I say lgtm with two tiny nits :) https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:190: ASSERT(m_highPriorityTaskCount >= 0); I missed this before but I think we need to assert on the return value of atomicSubtract instead. Otherwise TSan will yell at us :( https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:82: bool hasPendingHighPriorityWork(); Could you move these two functions above the data members? Also, the first one should probably be const.
https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:190: ASSERT(m_highPriorityTaskCount >= 0); On 2014/08/12 15:05:31, Sami wrote: > I missed this before but I think we need to assert on the return value of > atomicSubtract instead. Otherwise TSan will yell at us :( Done. https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/439923006/diff/370001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:82: bool hasPendingHighPriorityWork(); On 2014/08/12 15:05:31, Sami wrote: > Could you move these two functions above the data members? Also, the first one > should probably be const. Done.
OK, well you have my blessing. When sami is satisfied, land away. lgtm. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:59: Scheduler* scheduler = Scheduler::shared(); We should add a FIXME that this should be removed once we make shutdown deterministic. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:174: Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); Because this is a bit fragile (if someone called this method on another thread, for instance) I mgith explain what is keepign this safe. I might also add an ASSERT(isMainThread()) to the top of this method. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:221: return acquireLoad(&m_highPriorityTaskCount) != 0; We should add a (possibly large) comment here to explain why we chose to use barrier loads here and teh consquences of not. We believe we don't actually need to do this, that failing to read this bool corectly would just result in delaying high priority tasks a little, etc. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:151: if (m_reentrantCount <= 4 && Scheduler::shared()) { Why this magic value of 4? I would at least put this in a constant so we can share it between the 4 places its used? Do we really need to check that the scheduler still exists? https://codereview.chromium.org/439923006/diff/390001/Source/wtf/DoubleBuffer... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/390001/Source/wtf/DoubleBuffer... Source/wtf/DoubleBufferedDeque.h:13: template <typename T> class DoubleBufferedDeque { I might add a small comment as to what this classs is for. WebKit was comment-phobic, but we dont' have to be in blink. :)
https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:59: Scheduler* scheduler = Scheduler::shared(); On 2014/08/12 16:16:01, eseidel wrote: > We should add a FIXME that this should be removed once we make shutdown > deterministic. Done. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:174: Deque<TracedTask>& inputTasks = m_pendingInputTasks.swapBuffers(); On 2014/08/12 16:16:01, eseidel wrote: > Because this is a bit fragile (if someone called this method on another thread, > for instance) I mgith explain what is keepign this safe. > > I might also add an ASSERT(isMainThread()) to the top of this method. Done. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:221: return acquireLoad(&m_highPriorityTaskCount) != 0; On 2014/08/12 16:16:01, eseidel wrote: > We should add a (possibly large) comment here to explain why we chose to use > barrier loads here and teh consquences of not. > > We believe we don't actually need to do this, that failing to read this bool > corectly would just result in delaying high priority tasks a little, etc. Done. https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... File Source/platform/scheduler/SchedulerTest.cpp (right): https://codereview.chromium.org/439923006/diff/390001/Source/platform/schedul... Source/platform/scheduler/SchedulerTest.cpp:151: if (m_reentrantCount <= 4 && Scheduler::shared()) { On 2014/08/12 16:16:01, eseidel wrote: > Why this magic value of 4? I would at least put this in a constant so we can > share it between the 4 places its used? > > Do we really need to check that the scheduler still exists? Done. https://codereview.chromium.org/439923006/diff/390001/Source/wtf/DoubleBuffer... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/439923006/diff/390001/Source/wtf/DoubleBuffer... Source/wtf/DoubleBufferedDeque.h:13: template <typename T> class DoubleBufferedDeque { On 2014/08/12 16:16:01, eseidel wrote: > I might add a small comment as to what this classs is for. WebKit was > comment-phobic, but we dont' have to be in blink. :) Done.
Still lgtm. Just two minor comments. https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:60: // FIXME: This check should't be necessary, tasks should not outlive blink. Should we have an assert here too to match the function below? https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:175: // These locks guard against another thread posing input or compositor tasks while we swap the buffers. s/posing/posting/
https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:60: // FIXME: This check should't be necessary, tasks should not outlive blink. On 2014/08/13 10:28:43, Sami wrote: > Should we have an assert here too to match the function below? Done. https://codereview.chromium.org/439923006/diff/410001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:175: // These locks guard against another thread posing input or compositor tasks while we swap the buffers. On 2014/08/13 10:28:43, Sami wrote: > s/posing/posting/ Done.
The CQ bit was checked by alexclarke@google.com
The CQ bit was unchecked by alexclarke@google.com
The CQ bit was checked by alexclarke@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexclarke@chromium.org/439923006/450001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...) linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/41063) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/46281)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was checked by alexclarke@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexclarke@chromium.org/439923006/470001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by alexclarke@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexclarke@chromium.org/439923006/470001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_gn_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
Message was sent while issue was closed.
Committed patchset #25 (470001) as 180171 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
