Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
595023002:
Implement idle task scheduling. (Closed)
|
Created:
6 years, 2 months ago by rmcilroy Modified:
6 years, 2 months ago CC:
aandrey+blink_chromium.org, abarth-chromium, blink-reviews, blink-reviews-wtf_chromium.org, Hannes Payer (out of office), Mikhail, mkwst+moarreviews_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionImplement idle task scheduling.
Implements idle task scheduling in the Blink scheduler. These tasks are only scheduled after the compositor has committed a frame and have a deadline before the next frame is scheduled to begin.
BUG=397026
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183364
Patch Set 1 #
Total comments: 37
Patch Set 2 : Post idle tasks to the main thread run loop. #
Total comments: 5
Patch Set 3 : Change function names. #
Total comments: 3
Patch Set 4 : Avoid reposting idle tasks #
Total comments: 23
Patch Set 5 : Address Alex and Sami's comments #Patch Set 6 : Fix comment #
Total comments: 14
Patch Set 7 : Address Jochen's comments #Patch Set 8 : Fix whitespace #
Total comments: 2
Patch Set 9 : Rename estimatedNextBeginFrameSeconds #
Total comments: 2
Patch Set 10 : Move to OwnPtr #
Total comments: 2
Patch Set 11 : Add shutdown test. #Patch Set 12 : Tweak comment #Patch Set 13 : Fix Expect_EQ #
Messages
Total messages: 47 (9 generated)
rmcilroy@chromium.org changed reviewers: + picksi@chromium.org, skyostil@chromium.org
This isn't ready for review yet but I thought I'd send it to discuss how the code might look. Simon, as discussed, I've made it such that both the high and idle pending queues are queues of the same type, however I'm not convicted this is a great idea since it means that the TracedIdleTask needs to interigate the scheduler for the deadline, and it doesn't seem to gain much since the scheduler code to execute these two queues necessarily needs to be quite different. WDYT? Some other questions inline. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:13: // #include "public/web/WebBeginFrameArgs.h" Sami: I get a checkdeps error when I include this file, but need to do so to access the fields in WebBeginFrameArgs. Can we change the DEPS file to allow this include or do we need to move things around a bit? https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:116: m_currentFrameDeadlineSeconds = args.lastFrameTimeMonotonic + args.interval; This is the deadline used by the compositor implementation rather than the explicit deadline field in args. Should we use the deadline field instead? https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:65: double availableIdleTime = scheduler->getSecondsUntilFrameDeadline(); Simon: I'm not entirely convinced that it is worth making the TracedIdleTask subclass from TracedTask currently. There doesn't seem to be much benifit from both pendingTask queues being queue's of the same type since we need to execute them differently in the scheduler anyway (see Scheduler::executeIdleTasks()), and the requirement the TracedIdleTask::run() has no-args means that we need to call scheduler->getSecondsUntilFrameDeadline() from here as well as Scheduler::executeIdleTasks(), when otherwise it could be passed as an argument to this function. WDYT?
skyostil@chromium.org changed reviewers: + alexclarke@chromium.org
Thanks Ross, this is a great starting point. Let's figure out a way to mesh this with Alex's low priority task idea: https://codereview.chromium.org/540373002/ https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:13: // #include "public/web/WebBeginFrameArgs.h" On 2014/09/24 08:49:18, rmcilroy wrote: > Sami: I get a checkdeps error when I include this file, but need to do so to > access the fields in WebBeginFrameArgs. Can we change the DEPS file to allow > this include or do we need to move things around a bit? Right, we can't use the web/ headers from platform. I think we can just pass the necessary parameters to willBeginFrame() as separate doubles instead of using the struct. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:116: m_currentFrameDeadlineSeconds = args.lastFrameTimeMonotonic + args.interval; On 2014/09/24 08:49:18, rmcilroy wrote: > This is the deadline used by the compositor implementation rather than the > explicit deadline field in args. Should we use the deadline field instead? I think there are two different deadlines here: 1) the time we should start running idle tasks (i.e., args.deadline) and 2) the time we should stop running idle tasks in anticipation of new input. I think we should have both as explicit variables with good names to avoid confusing them. Looks like your patch uses the second type of deadline so it should base that on the interval like you are doing. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:121: if (getSecondsUntilFrameDeadline() > 0) { I don't think we want to run any tasks directly in this callback since the code that calls this also does other things. Let's post another task instead. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:106: double getSecondsUntilFrameDeadline() const; nit: drop the |get|, it's cleaner https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:135: DoubleBufferedDeque<OwnPtr<TracedTask> > m_pendingHighPriorityTasks; As of Nico's email to chromium-dev@ yesterday I think we can now finally write >> here :) https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:64: Scheduler* scheduler = Scheduler::shared(); Another note to self: we should keep |scheduler| as a member to avoid the global lookup. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:66: if (availableIdleTime > 0) { I think there's a race here where the scheduler can think it ran an idle task but in this function we actually bailed out => the task got dropped. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.h:22: class TracedTask { Note to self: this should probably be renamed as PendingTask. https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:21: void append(const PassType& value) Is this an optimization to avoid copies or something else?
Some comments! https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:273: } I wonder if the added complexity of swapping buffers and then re-posting the unprocessed tasks will turn out to be worth the hassle? I'm assuming that there might be a lot of idle tasks but only a few will get processed every frame, so there will be lots of shuffling going on. As part of my refactor-proposal I ended up creating a task queue that could return a task at a time, something like this would make the code much simpler... even if in some instances it would be slower. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:349: Do we actually need the result in seconds? Is it worth making another function deadlineHasPassed() that could be used more explicitly if we don't care about the actual value? Also, not sure if clamping the result to be non-negative is necessary... a -ve result still could be useful (to some future developer) and it's one less line of code. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:38: "src_func", m_location.functionName()); This function is confusingly named, how about updateTraceEvents() or something. Also AFAIK the TRACE_EVENT2 macro needs to be in the same scope as the execution of the task to correctly measure its duration... Sami will be able to confirm or deny this. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:58: } Do we need this ASSERT here? Might there be a situation were we create tasks ahead of time (e.g. dummy tasks or something)? Should the assert be in the run() function instead? https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:65: double availableIdleTime = scheduler->getSecondsUntilFrameDeadline(); I see what you mean. I wonder if there's the beginning of a pattern here though - different types of tasks have different conditions that allow their bulk-execution to even be considered; Idle tasks need sufficient time, HighPriority tasks need to have the correct policy setting (... etc?). Also having polymorphic tasks will be really helpful if we ever want to have a single point of task execution within the scheduler (which I'd favour as it aids understandability and debugging). It's certainly worth more discussion though. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:68: m_idleTask(availableIdleTime * 1000); Are we able to choose a time unit, seconds or milliseconds, rather than swapping between the two? Or are our hands tied by other APIs? https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:25: FWIW during my refactor I stopped using the DoubleBufferedDeque as hand-rolling my own double buffered deque was easy and allowed me to add more functionality to it without bloating this class. We should keep an eye on this if we find ourselves making more changes to this class.
Updated to post idle tasks to the main thread run loop. PTAL, thanks. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:13: // #include "public/web/WebBeginFrameArgs.h" On 2014/09/24 14:00:40, Sami wrote: > On 2014/09/24 08:49:18, rmcilroy wrote: > > Sami: I get a checkdeps error when I include this file, but need to do so to > > access the fields in WebBeginFrameArgs. Can we change the DEPS file to allow > > this include or do we need to move things around a bit? > > Right, we can't use the web/ headers from platform. I think we can just pass the > necessary parameters to willBeginFrame() as separate doubles instead of using > the struct. Done. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:116: m_currentFrameDeadlineSeconds = args.lastFrameTimeMonotonic + args.interval; On 2014/09/24 14:00:40, Sami wrote: > On 2014/09/24 08:49:18, rmcilroy wrote: > > This is the deadline used by the compositor implementation rather than the > > explicit deadline field in args. Should we use the deadline field instead? > > I think there are two different deadlines here: 1) the time we should start > running idle tasks (i.e., args.deadline) and 2) the time we should stop running > idle tasks in anticipation of new input. I think we should have both as explicit > variables with good names to avoid confusing them. > > Looks like your patch uses the second type of deadline so it should base that on > the interval like you are doing. Acknowledged. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:121: if (getSecondsUntilFrameDeadline() > 0) { On 2014/09/24 14:00:40, Sami wrote: > I don't think we want to run any tasks directly in this callback since the code > that calls this also does other things. Let's post another task instead. Sounds like a plan - done. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:273: } I've re-worked this completely such that we only ever take a single task off the pending idle queue at a time now. At worst that task will be reposted if it can't be executed, but there is no swapping buffers or reposting of many items. > I wonder if the added complexity of swapping buffers and then re-posting the > unprocessed tasks will turn out to be worth the hassle? I'm assuming that there > might be a lot of idle tasks but only a few will get processed every frame, so > there will be lots of shuffling going on. Actually I don't think the pending idle queue will be very large, probably only a few items in it most of the time I would think (but maybe I'm wrong). https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.cpp:349: On 2014/09/25 08:53:31, picksi1 wrote: > Do we actually need the result in seconds? Is it worth making another function > deadlineHasPassed() that could be used more explicitly if we don't care about > the actual value? > > Also, not sure if clamping the result to be non-negative is necessary... a -ve > result still could be useful (to some future developer) and it's one less line > of code. Reworked this so that we have canRunIdleTask() and currentFrameDeadline() instead. No clamping now since we never expose the remaining amount of allotted time. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:106: double getSecondsUntilFrameDeadline() const; On 2014/09/24 14:00:40, Sami wrote: > nit: drop the |get|, it's cleaner Done. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Sc... Source/platform/scheduler/Scheduler.h:135: DoubleBufferedDeque<OwnPtr<TracedTask> > m_pendingHighPriorityTasks; On 2014/09/24 14:00:40, Sami wrote: > As of Nico's email to chromium-dev@ yesterday I think we can now finally write > >> here :) Acknowledged. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.cpp (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:38: "src_func", m_location.functionName()); On 2014/09/25 08:53:31, picksi1 wrote: > This function is confusingly named, how about updateTraceEvents() or something. Changed to endFlowTraceEvent() (and only deals with the flow event now). > Also AFAIK the TRACE_EVENT2 macro needs to be in the same scope as the execution > of the task to correctly measure its duration... Sami will be able to confirm or > deny this. Yes you are right - oops. Moved to the appropriate scope. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:58: } On 2014/09/25 08:53:31, picksi1 wrote: > Do we need this ASSERT here? Might there be a situation were we create tasks > ahead of time (e.g. dummy tasks or something)? Should the assert be in the run() > function instead? I think these should only be created by the Scheduler, so this assert should pass I would have thought - not sure though, happy to move if either Sami or you think that it might fail in certain situations. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:64: Scheduler* scheduler = Scheduler::shared(); On 2014/09/24 14:00:40, Sami wrote: > Another note to self: we should keep |scheduler| as a member to avoid the global > lookup. Acknowledged. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:65: double availableIdleTime = scheduler->getSecondsUntilFrameDeadline(); On 2014/09/25 08:53:31, picksi1 wrote: > I see what you mean. I wonder if there's the beginning of a pattern here though > - different types of tasks have different conditions that allow their > bulk-execution to even be considered; Idle tasks need sufficient time, > HighPriority tasks need to have the correct policy setting (... etc?). > > Also having polymorphic tasks will be really helpful if we ever want to have a > single point of task execution within the scheduler (which I'd favour as it aids > understandability and debugging). > > It's certainly worth more discussion though. The fact that the new patch set passes a deadline rather than an allotted time makes me more amenable to this approach, since it avoids the need to call the expensive getSecondsUntilFrameDeadline() function twice (currentFrameDeadlineSeconds is a much cheaper function since it doesn't call Platform::monotonicallyIncreasingTime()). https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:66: if (availableIdleTime > 0) { On 2014/09/24 14:00:40, Sami wrote: > I think there's a race here where the scheduler can think it ran an idle task > but in this function we actually bailed out => the task got dropped. Your right - in the maybeExecuteIdleTask function returns true if it posted an IdleTask on the main thread, but that doesn't specify that the task will be executed (we can't know at that point). In either case though, the task will either be executed or reposted to the pending queue, so I think this should be good enough. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.cpp:68: m_idleTask(availableIdleTime * 1000); On 2014/09/25 08:53:31, picksi1 wrote: > Are we able to choose a time unit, seconds or milliseconds, rather than swapping > between the two? Or are our hands tied by other APIs? Change all APIs to seconds. https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/1/Source/platform/scheduler/Tr... Source/platform/scheduler/TracedTask.h:22: class TracedTask { On 2014/09/24 14:00:40, Sami wrote: > Note to self: this should probably be renamed as PendingTask. Acknowledged. https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:21: void append(const PassType& value) On 2014/09/24 14:00:40, Sami wrote: > Is this an optimization to avoid copies or something else? This is required to be able to create a DoubleBufferedDeque with OwnPtr or RefPtr because the append function needs to use the pass type (PassOwnPtr) rather than an OwnPtr (which wouldn't compile). It shouldn't make any difference on templated types which are not OwnPtr or RefPtr (PassType == T in those cases). https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:25: On 2014/09/25 08:53:31, picksi1 wrote: > FWIW during my refactor I stopped using the DoubleBufferedDeque as hand-rolling > my own double buffered deque was easy and allowed me to add more functionality > to it without bloating this class. We should keep an eye on this if we find > ourselves making more changes to this class. I would much prefer to fix this DoubleBufferedDeque such that it is useful rather than hand rolling our own if at all possible - is there any reason you couldn't add the functionality you needed here in your refactor?
Thanks! I've made a couple of comments about function names (my favourite subject!), https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:25: My changes felt against the spirit of the DoubleBufferedDeque - I was taking tasks off the 'hidden' deque. I also (incorrectly) thought this was a fundamental low-level class that would cause turmoil in the Blink masses if I'd changed it ... turns out that Alex wrote it a couple of weeks ago, so I should have been less scared. :) https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:302: } Is this function correctly named? It isn't actually executing the idle task - it's asking the main thread to try and run it at some future point. Does the canRunIdleTask() make sense here? Isn't this function only called if canRunIdleTask()==true, and the actual task will only be executed following a subsequent canRunIdleTask() check anyway. https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:312: Should this be called repostAtFront (etc) or something that makes clear what it's doing?
https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... File Source/wtf/DoubleBufferedDeque.h (right): https://codereview.chromium.org/595023002/diff/1/Source/wtf/DoubleBufferedDeq... Source/wtf/DoubleBufferedDeque.h:25: On 2014/09/30 09:13:51, picksi1 wrote: > My changes felt against the spirit of the DoubleBufferedDeque - I was taking > tasks off the 'hidden' deque. I also (incorrectly) thought this was a > fundamental low-level class that would cause turmoil in the Blink masses if I'd > changed it ... turns out that Alex wrote it a couple of weeks ago, so I should > have been less scared. :) Makes sense. You should never be scared of causing turmoil for the Blink masses though ;). https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:302: } On 2014/09/30 09:13:51, picksi1 wrote: > Is this function correctly named? It isn't actually executing the idle task - > it's asking the main thread to try and run it at some future point. Yes it's tricky to name - I originally had maybeScheduleIdleTask but felt that was confusing (since the task is already scheduled to be executed). I've renamed to maybePostMainThreadPendingIdleTask in order to fit with maybePostMainThreadPendingHighPriorityTaskRunner. WDYT? > Does the canRunIdleTask() make sense here? Isn't this function only called if > canRunIdleTask()==true, and the actual task will only be executed following a > subsequent canRunIdleTask() check anyway. No, this can be called from didCommitFrameToCompositor after the deadline for the next frame has already passed, or from MainThreadPendingIdleTaskRunner after it has run a task, which may have caused the current time to move passed the deadline. I think it makes most sense to check canRunIdleTask here explicitly rather than having to ensure we check it explicitly before calling this function. https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:312: On 2014/09/30 09:13:51, picksi1 wrote: > Should this be called repostAtFront (etc) or something that makes clear what > it's doing? Done.
https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/20001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:302: } maybePostMainThreadPendingHighPriorityTaskRunner() is a mouthful but is clear as to what it's doing! LGTM.
alexclarke@google.com changed reviewers: + alexclarke@google.com
https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:100: scheduler->repostIdleTaskAtFront(m_idleTask); It seems complex to pop and then repost the task if we can't run it. Why not do the pop here if scheduler->canRunIdleTask() is true?
https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:100: scheduler->repostIdleTaskAtFront(m_idleTask); On 2014/09/30 15:47:51, alexclarke wrote: > It seems complex to pop and then repost the task if we can't run it. Why not do > the pop here if scheduler->canRunIdleTask() is true? Yeah, that seems simpler. Just need to be careful about locking and re-entrancy.
rmcilroy@chromium.org changed reviewers: + jochen@chromium.org
+Jochen, could you review for Platform owners please. https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/40001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:100: scheduler->repostIdleTaskAtFront(m_idleTask); On 2014/09/30 16:14:47, Sami wrote: > On 2014/09/30 15:47:51, alexclarke wrote: > > It seems complex to pop and then repost the task if we can't run it. Why not > do > > the pop here if scheduler->canRunIdleTask() is true? > > Yeah, that seems simpler. Just need to be careful about locking and re-entrancy. Make sense, done.
https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:211: if (canRunIdleTask()) { Which threads will this function get called from? If it's only the Blink scheduler thread then this is fine (please add an assert if so), otherwise I'm not sure this is safe and I suspect it needs to be guarded by the mutex. Likewise willBeginFrame and didCommitFrameToCompositor should be too. I hope that doesn't open the possibility of deadlocking. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:244: bool Scheduler::runPendingIdleTaskIfCanRunIdleTask() nit: how about maybeRunPendingIdleTask? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:248: if (canRunIdleTask() && !m_pendingIdleTasks.isEmpty()) { While personally I like it your way, I'm under the impression it's more consistent with blink style to invert the if like so: if (!canRunIdleTask() || m_pendingIdleTasks.isEmpty()) { m_pendingTasksMutex.unlock(); return false; }
https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:211: if (canRunIdleTask()) { On 2014/10/03 09:59:19, alexclarke wrote: > Which threads will this function get called from? If it's only the Blink > scheduler thread then this is fine (please add an assert if so), otherwise I'm > not sure this is safe and I suspect it needs to be guarded by the mutex. > > Likewise willBeginFrame and didCommitFrameToCompositor should be too. I hope > that doesn't open the possibility of deadlocking. Looks like m_pendingIdleTasks is the only bit of cross-thread data here, right? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:247: m_pendingTasksMutex.lock(); Is there a way we could rewrite this to use a Locker? For example by having a takeFirstTask(m_pendingIdleTasks) helper that handles locking? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:353: return m_currentFrameDeadlineSeconds; ASSERT(isMainThread()); https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:358: return m_currentFrameCommitted && (m_currentFrameDeadlineSeconds > Platform::current()->monotonicallyIncreasingTime()); ASSERT(isMainThread()); https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:45: void postIdleTask(const TraceLocation&, const IdleTask&); // For non-critical tasks which may be reordered relative to other task types. Let's also mention here that idle tasks can be starved for arbitrarily long. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:89: bool runPendingIdleTaskIfCanRunIdleTask(); This is a mouthful. How about runPendintIdleTaskIfAllowed()? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:100: double currentFrameDeadlineSeconds() const; Please be explicit about which deadline this is talking about (currentFrameCompositingDeadline?) Or better yet: currentFrameDeadlineForIdleTasks()? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/TracedTask.h:57: typedef Function<void(double deadlineSeconds)> IdleTask; If we have this, do we need the similar typedef in Scheduler? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/TracedTask.h:63: friend class Scheduler; We're a friend of the Scheduler and they're a friend of us. This is a little too much sharing I think :) Can we have it go one way only? Or ideally by adding an interface for the thing that needs to be shared?
https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:211: if (canRunIdleTask()) { On 2014/10/03 10:39:21, Sami wrote: > On 2014/10/03 09:59:19, alexclarke wrote: > > Which threads will this function get called from? If it's only the Blink > > scheduler thread then this is fine (please add an assert if so), otherwise I'm > > not sure this is safe and I suspect it needs to be guarded by the mutex. > > > > Likewise willBeginFrame and didCommitFrameToCompositor should be too. I hope > > that doesn't open the possibility of deadlocking. > > Looks like m_pendingIdleTasks is the only bit of cross-thread data here, right? Right, mpendingIdleTasks is the only bit of cross-thread data here. I've added the check that this is only called on one thread. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:244: bool Scheduler::runPendingIdleTaskIfCanRunIdleTask() On 2014/10/03 09:59:19, alexclarke wrote: > nit: how about maybeRunPendingIdleTask? Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:247: m_pendingTasksMutex.lock(); On 2014/10/03 10:39:21, Sami wrote: > Is there a way we could rewrite this to use a Locker? For example by having a > takeFirstTask(m_pendingIdleTasks) helper that handles locking? I originally tried to write this to use a locker, however it wasn't really possible if I keep the isEmpty check since I would need to be able to return a NULL task (which I can't do unless I allocate on the heap), or pass a task to be filled in, which seems a bad design. Saying that, we shouldn't really need to check whether the queue is empty since this method will only be run if there was a task on the queue already, so I have rewritten it without this check, wdyt? https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:248: if (canRunIdleTask() && !m_pendingIdleTasks.isEmpty()) { On 2014/10/03 09:59:18, alexclarke wrote: > While personally I like it your way, I'm under the impression it's more > consistent with blink style to invert the if like so: > > if (!canRunIdleTask() || m_pendingIdleTasks.isEmpty()) { > m_pendingTasksMutex.unlock(); > return false; > } Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:353: return m_currentFrameDeadlineSeconds; On 2014/10/03 10:39:21, Sami wrote: > ASSERT(isMainThread()); Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.cpp:358: return m_currentFrameCommitted && (m_currentFrameDeadlineSeconds > Platform::current()->monotonicallyIncreasingTime()); On 2014/10/03 10:39:21, Sami wrote: > ASSERT(isMainThread()); Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:45: void postIdleTask(const TraceLocation&, const IdleTask&); // For non-critical tasks which may be reordered relative to other task types. On 2014/10/03 10:39:21, Sami wrote: > Let's also mention here that idle tasks can be starved for arbitrarily long. Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:89: bool runPendingIdleTaskIfCanRunIdleTask(); On 2014/10/03 10:39:22, Sami wrote: > This is a mouthful. How about runPendintIdleTaskIfAllowed()? Made it maybeRunPendingIdleTask on Alex's suggestion. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/Scheduler.h:100: double currentFrameDeadlineSeconds() const; On 2014/10/03 10:39:21, Sami wrote: > Please be explicit about which deadline this is talking about > (currentFrameCompositingDeadline?) Or better yet: > currentFrameDeadlineForIdleTasks()? Done. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/TracedTask.h:57: typedef Function<void(double deadlineSeconds)> IdleTask; On 2014/10/03 10:39:22, Sami wrote: > If we have this, do we need the similar typedef in Scheduler? We do if we want people to be able to postIdleTask to the scheduler with only Scheduler.h included. It's the same as the typedef in TracedStandardTask above which is also in Scheduler.h. https://codereview.chromium.org/595023002/diff/60001/Source/platform/schedule... Source/platform/scheduler/TracedTask.h:63: friend class Scheduler; On 2014/10/03 10:39:22, Sami wrote: > We're a friend of the Scheduler and they're a friend of us. This is a little too > much sharing I think :) Can we have it go one way only? Or ideally by adding an > interface for the thing that needs to be shared? Removed TracedIdleTask as a friend of Scheduler and made Scheduler::currentFrameDeadlineSeconds public. I think this is OK to be public, but if you prefer an interface let me know.
https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:92: scheduler->runPendingHighPriorityTasksIfInCompositorPriority(); if we're in compositor priority, why run an idle task next? https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:23: public: nit. WTF_MAKE_NONCOPYABLE(TracedTask) https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:31: TraceLocation m_location; please add setters and getters instead of exposing the members (even protected) https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:48: friend class Scheduler; why does the Scheduler need to be a friend?
https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:92: scheduler->runPendingHighPriorityTasksIfInCompositorPriority(); On 2014/10/06 07:33:19, jochen wrote: > if we're in compositor priority, why run an idle task next? Right, this call could be removed from here and we could have maybeRunPendingIdleTask() bail out if there's pending high priority work. https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:366: return m_currentFrameCommitted && (m_currentFrameDeadlineSeconds > Platform::current()->monotonicallyIncreasingTime()); To jochen's point, should we be checking !shouldYieldForHighPriorityWork() here too? https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:66: double currentFrameDeadlineForIdleTasks() const; The name makes it sound like this is a deadline but the description sounds like it gives the time *until* the deadline. Please fix the one that is wrong :)
https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:92: scheduler->runPendingHighPriorityTasksIfInCompositorPriority(); On 2014/10/06 09:48:30, Sami wrote: > On 2014/10/06 07:33:19, jochen wrote: > > if we're in compositor priority, why run an idle task next? > > Right, this call could be removed from here and we could have > maybeRunPendingIdleTask() bail out if there's pending high priority work. The original reason I had this here was in case we change to compositor priority (or a high prio task is posted) in-between the MainThreadPendingIdleTaskRunner being posted on the run loop and it being run. I just talked to Sami and we decided just to make CanRunIdleTask return false if when we should yield to high priority tasks (so this task wouldn't get posted). https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:366: return m_currentFrameCommitted && (m_currentFrameDeadlineSeconds > Platform::current()->monotonicallyIncreasingTime()); On 2014/10/06 09:48:30, Sami wrote: > To jochen's point, should we be checking !shouldYieldForHighPriorityWork() here > too? Done. https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.h (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/Scheduler.h:66: double currentFrameDeadlineForIdleTasks() const; On 2014/10/06 09:48:30, Sami wrote: > The name makes it sound like this is a deadline but the description sounds like > it gives the time *until* the deadline. Please fix the one that is wrong :) Comment was unclear - fixed, thanks. https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:23: public: On 2014/10/06 07:33:19, jochen wrote: > nit. WTF_MAKE_NONCOPYABLE(TracedTask) I can't make this NONCOPYABLE due to the way it is stored in Deques. Sami - should we be storing OwnPtr<TracedTask> in the Deques (as in my 1'st patch-set on this CL), rather than copying TracedTask whenever we add / remove from the queues? https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:31: TraceLocation m_location; On 2014/10/06 07:33:19, jochen wrote: > please add setters and getters instead of exposing the members (even protected) Done. https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:48: friend class Scheduler; On 2014/10/06 07:33:19, jochen wrote: > why does the Scheduler need to be a friend? Because the constructor is private. Happy to make this public and remove the friend statement, but I was just being consistent with how TracedTask was previously defined.
https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... File Source/platform/scheduler/TracedTask.h (right): https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... Source/platform/scheduler/TracedTask.h:23: public: On 2014/10/06 11:29:42, rmcilroy wrote: > On 2014/10/06 07:33:19, jochen wrote: > > nit. WTF_MAKE_NONCOPYABLE(TracedTask) > > I can't make this NONCOPYABLE due to the way it is stored in Deques. Sami - > should we be storing OwnPtr<TracedTask> in the Deques (as in my 1'st patch-set > on this CL), rather than copying TracedTask whenever we add / remove from the > queues? Note if we have a *lot* of TracedTasks then the overhead of OwnPtr<TracedTask> might be unfortunate.
On 2014/10/06 11:36:11, alexclarke wrote: > https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... > File Source/platform/scheduler/TracedTask.h (right): > > https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... > Source/platform/scheduler/TracedTask.h:23: public: > On 2014/10/06 11:29:42, rmcilroy wrote: > > On 2014/10/06 07:33:19, jochen wrote: > > > nit. WTF_MAKE_NONCOPYABLE(TracedTask) > > > > I can't make this NONCOPYABLE due to the way it is stored in Deques. Sami - > > should we be storing OwnPtr<TracedTask> in the Deques (as in my 1'st patch-set > > on this CL), rather than copying TracedTask whenever we add / remove from the > > queues? > > Note if we have a *lot* of TracedTasks then the overhead of OwnPtr<TracedTask> > might be unfortunate. base::TaskQueue also copies tasks instead of using refcounting, and we ended up doing the same here. I'd err on the side of avoiding memory allocations in this case since TracedTask is designed to be cheaply copyable.
On 2014/10/06 11:44:55, Sami wrote: > On 2014/10/06 11:36:11, alexclarke wrote: > > > https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... > > File Source/platform/scheduler/TracedTask.h (right): > > > > > https://codereview.chromium.org/595023002/diff/100001/Source/platform/schedul... > > Source/platform/scheduler/TracedTask.h:23: public: > > On 2014/10/06 11:29:42, rmcilroy wrote: > > > On 2014/10/06 07:33:19, jochen wrote: > > > > nit. WTF_MAKE_NONCOPYABLE(TracedTask) > > > > > > I can't make this NONCOPYABLE due to the way it is stored in Deques. Sami - > > > should we be storing OwnPtr<TracedTask> in the Deques (as in my 1'st > patch-set > > > on this CL), rather than copying TracedTask whenever we add / remove from > the > > > queues? > > > > Note if we have a *lot* of TracedTasks then the overhead of OwnPtr<TracedTask> > > might be unfortunate. > > base::TaskQueue also copies tasks instead of using refcounting, and we ended up > doing the same here. I'd err on the side of avoiding memory allocations in this > case since TracedTask is designed to be cheaply copyable. Ack, will do. We should keep an eye on how often we copy things between queues this doesn't become an issue. Any other comments?
/me complaining about names again. https://codereview.chromium.org/595023002/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:137: void Scheduler::willBeginFrame(double frameDeadlineSeconds) Please call this something else than a deadline because it's different from the deadline we get in BeginFrameArgs. How about something like estimatedNextBeginFrameSeconds? Also the members should probably also be called something like m_currentIdleDeadlineSeconds.
https://codereview.chromium.org/595023002/diff/140001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/140001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:137: void Scheduler::willBeginFrame(double frameDeadlineSeconds) On 2014/10/06 13:49:42, Sami wrote: > Please call this something else than a deadline because it's different from the > deadline we get in BeginFrameArgs. How about something like > estimatedNextBeginFrameSeconds? Also the members should probably also be called > something like m_currentIdleDeadlineSeconds. Renamed arg and also member to (m_)estimatedNextBeginFrameSeconds - I think this is clearer and is more correct given that we might end up giving idle tasks shorter deadlines than this when in compositor mode.
Great, LGTM!
+hpayer Any more comments Jochen?
I'd rather not have friends. The tasks should have a static PassOwnPtr<FooTask> Create() method. OwnPtr supports move semantics, so it should actually be less expensive to have containers full of OwnPtrs
https://codereview.chromium.org/595023002/diff/160001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/595023002/diff/160001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1821: Scheduler::shared()->willBeginFrame( nit. no 80 char limit in blink
> I'd rather not have friends. > > > The tasks should have a static PassOwnPtr<FooTask> Create() method. OwnPtr > supports move semantics, so it should actually be less expensive to have > containers full of OwnPtrs Done. Also moved TracedTask to an internal namespace on Sami's suggestion to make it obvious that these should only be used by the Scheduler (since the constructors are no longer private). https://codereview.chromium.org/595023002/diff/160001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/595023002/diff/160001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1821: Scheduler::shared()->willBeginFrame( On 2014/10/07 11:20:38, jochen wrote: > nit. no 80 char limit in blink Done.
Also synced to pick up Sami's most recent changes.
Thanks Ross. https://codereview.chromium.org/595023002/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:88: // FIXME: This check shouldn't be necessary, tasks should not outlive blink. I don't think this comment is accurate any longer. To prove it, could you add a new entry to SchedulerTest.TestTasksRunAfterShutdown for idle tasks?
lgtm, thanks
https://codereview.chromium.org/595023002/diff/180001/Source/platform/schedul... File Source/platform/scheduler/Scheduler.cpp (right): https://codereview.chromium.org/595023002/diff/180001/Source/platform/schedul... Source/platform/scheduler/Scheduler.cpp:88: // FIXME: This check shouldn't be necessary, tasks should not outlive blink. On 2014/10/07 14:17:34, Sami wrote: > I don't think this comment is accurate any longer. To prove it, could you add a > new entry to SchedulerTest.TestTasksRunAfterShutdown for idle tasks? Done.
Thanks for adding the test. LGTM!
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/595023002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/595023002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/26145)
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/595023002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as 183364 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
