Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1130413003:
Schedule garbage collection on worker threads using idle tasks (Closed)
|
Created:
5 years, 7 months ago by Sami Modified:
5 years, 7 months ago CC:
blink-reviews, falken, horo+watch_chromium.org, kinuko+worker_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSchedule garbage collection on worker threads using idle tasks
Original patch by Alex Clarke <alexclarke@chromium.org>;.
BUG=463143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195243
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195402
Patch Set 1 #
Total comments: 2
Patch Set 2 : Style tweak. #Patch Set 3 : Rebased. #
Total comments: 2
Patch Set 4 : Fix locking in test. #
Total comments: 3
Patch Set 5 : Remove overly specific tests. #Patch Set 6 : Remove idle task wrapper. #Patch Set 7 : Rebased. #
Messages
Total messages: 25 (8 generated)
skyostil@chromium.org changed reviewers: + jochen@chromium.org, rmcilroy@chromium.org
Jochen, PTAL.
lgtm
alexclarke@chromium.org changed reviewers: + alexclarke@chromium.org
https://codereview.chromium.org/1130413003/diff/1/Source/core/workers/WorkerT... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/1/Source/core/workers/WorkerT... Source/core/workers/WorkerThread.cpp:378: m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new WorkerThreadIdleTask(this)); I wonder how this is going to interact with the quiesence detection. If that works properly with the shared timer then landing this seperatly is a very good idea. Originally I wasn't convinced it was possible to split this off but that was before the quiesence detection work.
https://codereview.chromium.org/1130413003/diff/1/Source/core/workers/WorkerT... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/1/Source/core/workers/WorkerT... Source/core/workers/WorkerThread.cpp:378: m_webScheduler->postIdleTaskAfterWakeup(FROM_HERE, new WorkerThreadIdleTask(this)); On 2015/05/12 08:42:14, alexclarke1 wrote: > I wonder how this is going to interact with the quiesence detection. If that > works properly with the shared timer then landing this seperatly is a very good > idea. Originally I wasn't convinced it was possible to split this off but that > was before the quiesence detection work. I think this should Just Work(tm), because the shared timer currently gets posted into the default task queue, which is being monitored for quiescence. IIRC we originally ran into problems because we were trying to exactly replicate the current logic of avoiding idle work if the next timer is firing within 300 ms.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1130413003/#ps40001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130413003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195243
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1128093003/ by skyostil@chromium.org. The reason for reverting is: Looks like the new tests are flaky on Windows, e.g: http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/29682.
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:509: m_webScheduler->postIdleTask(FROM_HERE, new WorkerThreadIdleTask(this)); We already have too many ways to post a task (https://code.google.com/p/chromium/issues/detail?id=486573). Would it be possible to hide this behind WebThread too?
https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:509: m_webScheduler->postIdleTask(FROM_HERE, new WorkerThreadIdleTask(this)); On 2015/05/12 19:10:44, sadrul wrote: > We already have too many ways to post a task > (https://code.google.com/p/chromium/issues/detail?id=486573). Would it be > possible to hide this behind WebThread too? This is already getting posted through the WebThread (WebThread::scheduler()->postIdleTask). Or did you mean moving the scheduler posting entrypoints to the WebThread? We decided to add the more specialized ones to WebScheduler instead of WebThread since not everyone needs to deal with them.
On 2015/05/12 19:15:43, Sami wrote: > https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... > File Source/core/workers/WorkerThread.cpp (right): > > https://codereview.chromium.org/1130413003/diff/40001/Source/core/workers/Wor... > Source/core/workers/WorkerThread.cpp:509: > m_webScheduler->postIdleTask(FROM_HERE, new WorkerThreadIdleTask(this)); > On 2015/05/12 19:10:44, sadrul wrote: > > We already have too many ways to post a task > > (https://code.google.com/p/chromium/issues/detail?id=486573). Would it be > > possible to hide this behind WebThread too? > > This is already getting posted through the WebThread > (WebThread::scheduler()->postIdleTask). Or did you mean moving the scheduler > posting entrypoints to the WebThread? The latter, yeah. > We decided to add the more specialized > ones to WebScheduler instead of WebThread since not everyone needs to deal with > them. Or maybe remove post-tasking from WebThread and move all of that into WebScheduler? Having more than one thing be responsible for posting a task seems potentially a bit confusing to me.
kinuko@chromium.org changed reviewers: + kinuko@chromium.org
On 2015/05/13 04:45:21, sadrul wrote: > Or maybe remove post-tasking from WebThread and move all of that into > WebScheduler? Having more than one thing be responsible for posting a task seems > potentially a bit confusing to me. Not all threads (need to) go through scheduler, I doubt if we want to do so. https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:311: // can't use WTF::bind because the type-erasure for member function pointers with parameters is broken. Could you give a bit more details about what's broken in WTF::bind? (This one looks fine, but arbitrarily mixing up base::Bind and WTF::bind in the same code base could be dangerous and I want to discourage doing so until we have some guideline. Context: go/thread-safe-blink)
I've now taken out the tests which were basically hardcoded against the scheduling policy we have on the Chromium side (which is already tested there). I've left a couple of tests with more generous scheduling constraints that check that basic idle gc is working. > On 2015/05/13 04:45:21, sadrul wrote: > > Or maybe remove post-tasking from WebThread and move all of that into > > WebScheduler? Having more than one thing be responsible for posting a task > seems > > potentially a bit confusing to me. > > Not all threads (need to) go through scheduler, I doubt if we want to do so. I agree. postTask and postDelayedTask are pretty universal concepts. If you want more specific task types then it makes sense that you should be using a more specialized interface (WebScheduler). In any case if we want to shuffle things around let's do that in another patch. https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:311: // can't use WTF::bind because the type-erasure for member function pointers with parameters is broken. On 2015/05/13 06:10:52, kinuko wrote: > Could you give a bit more details about what's broken in WTF::bind? (This one > looks fine, but arbitrarily mixing up base::Bind and WTF::bind in the same code > base could be dangerous and I want to discourage doing so until we have some > guideline. Context: go/thread-safe-blink) This part was written by Alex so I needed to reverse engineer it a bit. I think the problem was that WTF::bind doesn't derive unbound variables automatically like base::Bind, e.g.: void func(int x) { ... } (*WTF::bind(&func))(123) // doesn't work because bind returns a Function<void()>, not Function<void(int)>. (*WTF::bind<int>(&func))(123) // works. I've now taken out this wrapper and replaced it with a bind<double>.
https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/1130413003/diff/60001/Source/core/workers/Wor... Source/core/workers/WorkerThread.cpp:311: // can't use WTF::bind because the type-erasure for member function pointers with parameters is broken. On 2015/05/13 17:54:19, Sami wrote: > On 2015/05/13 06:10:52, kinuko wrote: > > Could you give a bit more details about what's broken in WTF::bind? (This one > > looks fine, but arbitrarily mixing up base::Bind and WTF::bind in the same > code > > base could be dangerous and I want to discourage doing so until we have some > > guideline. Context: go/thread-safe-blink) > > This part was written by Alex so I needed to reverse engineer it a bit. I think > the problem was that WTF::bind doesn't derive unbound variables automatically > like base::Bind, e.g.: > > void func(int x) { ... } > > (*WTF::bind(&func))(123) // doesn't work because bind returns a > Function<void()>, not Function<void(int)>. > > (*WTF::bind<int>(&func))(123) // works. > > I've now taken out this wrapper and replaced it with a bind<double>. Ah interesting. Thanks for the detail!
lgtm
lgtm
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1130413003/#ps10004 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130413003/10004
Message was sent while issue was closed.
Committed patchset #7 (id:10004) as https://src.chromium.org/viewvc/blink?view=rev&revision=195402 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
