Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
507873003:
[Blink-Worker] WorkerThread fires idleHandler only at the end of processing all tasks. (Closed)
|
Created:
6 years, 3 months ago by Mayur Kankanwadi Modified:
3 years, 3 months ago CC:
blink-reviews, falken, horo+watch_chromium.org, kinuko+worker_chromium.org, lgombos Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[Blink-Worker] WorkerThread fires idleHandler only at the end of processing all tasks.
This CL allows Workerthread to keep a count of tasks queued up. Once all the tasks
are run, only then the idleHandler is triggered once to GC. The idleHandler is
then only trigerred once any new task is posted.
BUG=401057
Patch Set 1 #Patch Set 2 : Removed spurious new line deletion. #
Total comments: 13
Patch Set 3 : Review comments incorporated. #Patch Set 4 : Rebased on top of shared timer duplicate task CL. #Patch Set 5 : WIP upload. Most of the logic implemented, but still firing too many idlehandlers. #Patch Set 6 : Implemented new idleHandler firing logic. #
Total comments: 11
Patch Set 7 : Updated logic to cancel events and other nits. #Messages
Total messages: 31 (7 generated)
mayurk.vk@samsung.com changed reviewers: + jochen@chromium.org
Hi, This is the implementation which avoids firing idlehandler routinely. Please review this. Regards.
some initial comments. While trying to phrase my comments and reading through the code, I discovered that the shared timer we use on the worker thread is broken. Would you be willing to change this? The issue is that if the timer is first start with say 2s and then another time in 1s is created, we don't cancel the task for the 2s timer. If you compare this to the implementation in base/timer.h we need to have a way to cancel a previous job before posting a new one. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:171: if (!workerThread.decrementAndReturnTaskCount() && !workerThread.isIdleHandlerTaskFiredOnce()) maybe teach WorkerThreadTask that it's an idle task instead of always decrementing the task count? then you can't get negative in decrementAndReturnTaskCount() https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:424: m_isIdleHandlerTask = false; instead of using m_isIdleHandlerTask, this method should just directly create and post the task on m_thread. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:448: void WorkerThread::decrementTaskCount() this method isn't used anywhere? https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:459: if (!m_tasksCount) see my comment about not invoking this for idle tasks https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:465: unsigned WorkerThread::taskCount() not used anywhere? https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.h:97: unsigned decrementAndReturnTaskCount(); those should all be private
On 2014/09/02 10:14:14, jochen wrote: > some initial comments. > > While trying to phrase my comments and reading through the code, I discovered > that the shared timer we use on the worker thread is broken. Would you be > willing to change this? > > The issue is that if the timer is first start with say 2s and then another time > in 1s is created, we don't cancel the task for the 2s timer. If you compare this > to the implementation in base/timer.h we need to have a way to cancel a previous > job before posting a new one. Yes, I can surely take this up. Should we create a new bug for it or just fix it within this bug? As for the review comments will check them and incorporate the required changes. Thanks.
On 2014/09/02 at 14:03:21, mayurk.vk wrote: > On 2014/09/02 10:14:14, jochen wrote: > > some initial comments. > > > > While trying to phrase my comments and reading through the code, I discovered > > that the shared timer we use on the worker thread is broken. Would you be > > willing to change this? > > > > The issue is that if the timer is first start with say 2s and then another time > > in 1s is created, we don't cancel the task for the 2s timer. If you compare this > > to the implementation in base/timer.h we need to have a way to cancel a previous > > job before posting a new one. > > Yes, I can surely take this up. Should we create a new bug for it or just fix it within this bug? > As for the review comments will check them and incorporate the required changes. > Thanks. I think it should be fixed in a separate CL
Incorporated the comments. Will upload the new patch soon. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:171: if (!workerThread.decrementAndReturnTaskCount() && !workerThread.isIdleHandlerTaskFiredOnce()) On 2014/09/02 10:14:14, jochen wrote: > maybe teach WorkerThreadTask that it's an idle task instead of always > decrementing the task count? then you can't get negative in > decrementAndReturnTaskCount() Done. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:424: m_isIdleHandlerTask = false; On 2014/09/02 10:14:14, jochen wrote: > instead of using m_isIdleHandlerTask, this method should just directly create > and post the task on m_thread. Done. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:439: bool callGCAgain = !m_workerGlobalScope->idleNotification(); This(bool callGCAgain) was an extra definition, with a good scope for subtle bugs. Removed this as well. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:448: void WorkerThread::decrementTaskCount() On 2014/09/02 10:14:14, jochen wrote: > this method isn't used anywhere? True.Removed it. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:459: if (!m_tasksCount) On 2014/09/02 10:14:14, jochen wrote: > see my comment about not invoking this for idle tasks Done. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.cpp:465: unsigned WorkerThread::taskCount() On 2014/09/02 10:14:14, jochen wrote: > not used anywhere? True.Removed it. https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/507873003/diff/20001/Source/core/workers/Work... Source/core/workers/WorkerThread.h:97: unsigned decrementAndReturnTaskCount(); On 2014/09/02 10:14:14, jochen wrote: > those should all be private These functions are accessed from within the WorkerThreadTask, hence they need to be public.
Hi jochen, Please review the latest patch. Regards.
can we first fix the shared timer before proceeding with this patch?
On 2014/09/04 10:22:23, jochen wrote: > can we first fix the shared timer before proceeding with this patch? Hi jochen, I have created an issue for the timer issue: crbug.com/410786 I have some doubts regarding this,we can continue the discussion on this issue. Regards.
now that the timer issue is fixed, should we continue on this CL?
On 2014/10/07 14:32:36, jochen wrote: > now that the timer issue is fixed, should we continue on this CL? Hi jochen, Was finally able to rebase properly. PTAL at the changes done. Thanks.
jochen@chromium.org changed reviewers: + rmcilroy@chromium.org
+ross we're trying to make the scheduling of the idle task on a worker thread a bit smarter
so here's what I'd like to have on a high level. when the last task on the message loop was executed (T2 in the ascii art), and nothing else is on it for say, the next 10s, then after 10s start running idle tasks (I1 & I2) until all garbage is collected. +----+----+ +----+----+ | T1 | T2 |....10s pause ...| I1 | I2 |..... completely idle +----+----+ +----+----+ we do idle notifications of 1s until either no more garbage exists, or there's another task that was scheduled meanwhile, or the next timer is <1s in the future. does that make sense? it would be nice if we could avoid using a mutex for the counter, use atomics instead. Also, we should avoid changing the public interface of WorkerThread. Maybe the task class that has to access the counter methods can be a private class of WorkerThread, so it can access private methods?
Hi Jochen, I have updated the idleHandler firing logic as per your suggestions. PTAL. Thanks.
hum, so now if I post a task at 0s, it'll schedule a idle task at 10s after it completes. if then another task comes in at 5s, it won't do schedule a new idle task. but then the idle task is executed 5s after the last task, not 10s. I guess we have to do something similar as for the timer and cancel the idle task if it's too near in the future, say, if it's less than 7s in the future, and post a new one. wdyt? https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:209: && !m_workerThread.m_IdleHandlerTaskPosted) nit. put on previous line https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:466: OwnPtr<WorkerThreadTask> idleHandlerTask = WorkerThreadTask::create(*this, nit, put on one line https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:468: idleHandlerTask.get()->setIsIdleHandlerTask(true); should work without get() https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:481: m_IdleHandlerTaskPosted = false; nit. move this up outside the if() https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:496: queueUpIdleHandlerNow(delay); should be 0 https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:498: m_IdleHandlerTaskPosted = false; not needed anymore after the one above is mvoed https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.h (right): https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.h:64: class WorkerThreadTask : public blink::WebThread::Task { nit. move down to the provide section. it's also enough to forward declare the class (just class WorkerThreadTask;)
On 2014/10/14 13:46:03, jochen (OOO) wrote: > hum, so now if I post a task at 0s, it'll schedule a idle task at 10s after it > completes. > > if then another task comes in at 5s, it won't do schedule a new idle task. > > but then the idle task is executed 5s after the last task, not 10s. > > I guess we have to do something similar as for the timer and cancel the idle > task if it's too near in the future, say, if it's less than 7s in the future, > and post a new one. wdyt? > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > File Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:209: && > !m_workerThread.m_IdleHandlerTaskPosted) > nit. put on previous line > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:466: OwnPtr<WorkerThreadTask> > idleHandlerTask = WorkerThreadTask::create(*this, > nit, put on one line > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:468: > idleHandlerTask.get()->setIsIdleHandlerTask(true); > should work without get() > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:481: m_IdleHandlerTaskPosted = false; > nit. move this up outside the if() > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:496: queueUpIdleHandlerNow(delay); > should be 0 > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:498: m_IdleHandlerTaskPosted = false; > not needed anymore after the one above is mvoed > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > File Source/core/workers/WorkerThread.h (right): > > https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.h:64: class WorkerThreadTask : public > blink::WebThread::Task { > nit. move down to the provide section. > > it's also enough to forward declare the class (just class WorkerThreadTask;) Hi jochen, What would be the effect of letting the gc fire too close to the completion of the last task? Would it be too aggressive GC? Regards.
https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:466: OwnPtr<WorkerThreadTask> idleHandlerTask = WorkerThreadTask::create(*this, On 2014/10/14 13:46:03, jochen (OOO) wrote: > nit, put on one line Done. https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:468: idleHandlerTask.get()->setIsIdleHandlerTask(true); On 2014/10/14 13:46:03, jochen (OOO) wrote: > should work without get() Done. https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:481: m_IdleHandlerTaskPosted = false; On 2014/10/14 13:46:03, jochen (OOO) wrote: > nit. move this up outside the if() Done. https://codereview.chromium.org/507873003/diff/100001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:498: m_IdleHandlerTaskPosted = false; On 2014/10/14 13:46:03, jochen (OOO) wrote: > not needed anymore after the one above is mvoed Done.
Hi jochen, I have updated the logic as per your comments. PTAL. Thanks.
jochen@chromium.org changed reviewers: + sullivan@chromium.org
+sullivan who I talked to about this patch last week. Sorry for the long delay in review, I'll try to get to this asap, but I can't yet promise when btw, did you see https://codereview.chromium.org/690703002 ?
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Hi Mayur, It looks like your patch has some overlap with the idle GC tasks we've recently added to the Blink scheduler: https://docs.google.com/a/chromium.org/document/d/1bXcZ45iCr9NPP6UDbY57RCKgSn... I think it would be better to let the scheduler decide a good time for GC instead of trying to figure it out from the task queue state. Could you help us figure out how the scheduler should be extended to achieve this?
The CQ bit was checked by alexclarke@chromium.org
The CQ bit was unchecked by alexclarke@chromium.org
On 2014/11/10 11:42:37, Sami wrote: > Hi Mayur, > > It looks like your patch has some overlap with the idle GC tasks we've recently > added to the Blink scheduler: > https://docs.google.com/a/chromium.org/document/d/1bXcZ45iCr9NPP6UDbY57RCKgSn... > > I think it would be better to let the scheduler decide a good time for GC > instead of trying to figure it out from the task queue state. Could you help us > figure out how the scheduler should be extended to achieve this? Sami: Just in case you don't realise, this is for Worker threads - the scheduler currently only deals with the main thread. Do we want to extend the scheduler to deal with other threads?
On 2014/11/10 11:57:55, rmcilroy wrote: > Sami: Just in case you don't realise, this is for Worker threads - the scheduler > currently only deals with the main thread. Do we want to extend the scheduler > to deal with other threads? Thanks for pointing that out. I initially read this as applying to both the main and worker threads. Regardless, we probably want to investigate scheduling worker threads too.
On 2014/11/04 at 06:36:40, mayurk.vk wrote: > Hi jochen, > What would be the effect of letting the gc fire too close to the completion of the last task? > Would it be too aggressive GC? > Regards. GC can take some time, and if we're not idle for an extended period of time, we have to assume that the worker is busy doing something else.
On 2014/11/10 13:57:03, jochen (slow) wrote: > On 2014/11/04 at 06:36:40, mayurk.vk wrote: > > Hi jochen, > > What would be the effect of letting the gc fire too close to the completion of > the last task? > > Would it be too aggressive GC? > > Regards. > > GC can take some time, and if we're not idle for an extended period of time, we > have to assume that the worker is busy doing something else. Hi All, Sorry for the delay in updates. I am currently stuck with some other matters. Hope to resolve them soon. So please bear with me. Best regards, Mayur Kankanwadi.
Description was changed from ========== [Blink-Worker] WorkerThread fires idleHandler only at the end of processing all tasks. This CL allows Workerthread to keep a count of tasks queued up. Once all the tasks are run, only then the idleHandler is triggered once to GC. The idleHandler is then only trigerred once any new task is posted. BUG=401057 ========== to ========== [Blink-Worker] WorkerThread fires idleHandler only at the end of processing all tasks. This CL allows Workerthread to keep a count of tasks queued up. Once all the tasks are run, only then the idleHandler is triggered once to GC. The idleHandler is then only trigerred once any new task is posted. BUG=401057 ==========
alexclarke@chromium.org changed reviewers: - alexclarke@chromium.org |
