Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(92)

Issue 987193002: Redirect the MessageLoop's task runner to the renderer scheduler (Closed)

Created:
5 years, 9 months ago by Sami
Modified:
5 years, 5 months ago
CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, dcheng, aandrey+blink_chromium.org, ben+mojo_chromium.org, tim+watch_chromium.org, nona+watch_chromium.org, kalyank, mlamouri+watch-content_chromium.org, mikhal+watch_chromium.org, pam+watch_chromium.org, chirantan+watch_chromium.org, maniscalco+watch_chromium.org, avayvod+watch_chromium.org, estade+watch_chromium.org, wjia+watch_chromium.org, pfeldman, vandebo (ex-Chrome), viettrungluu+watch_chromium.org, erikwright (departed), gavinp+memory_chromium.org, sievers+watch_chromium.org, kinuko+watch, mlamouri+watch-geolocation_chromium.org, Aaron Boodman, lcwu+watch_chromium.org, jam, eme-reviews_chromium.org, chromium-apps-reviews_chromium.org, android-webview-reviews_chromium.org, Greg Billock, jsbell+serviceworker_chromium.org, penghuang+watch_chromium.org, nhiroki, asvitkine+watch_chromium.org, Michael van Ouwerkerk, gunsch+watch_chromium.org, mkwst+watchlist-passwords_chromium.org, hclam+watch_chromium.org, michaelpg+watch-options_chromium.org, Lei Zhang, serviceworker-reviews, yurys, horo+watch_chromium.org, yusukes+watch_chromium.org, zea+watch_chromium.org, browser-components-watch_chromium.org, yzshen+watch_chromium.org, yukishiino+watch_chromium.org, dgrogan, tracing+reviews_chromium.org, miu+watch_chromium.org, extensions-reviews_chromium.org, pvalenzuela+watch_chromium.org, dzhioev+watch_chromium.org, abarth-chromium, hguihot+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, erikwright+watch_chromium.org, imcheng+watch_chromium.org, jasonroberts+watch_google.com, oshima+watch_chromium.org, piman+watch_chromium.org, wfh+watch_chromium.org, maxbogue+watch_chromium.org, tommycli, cmumford, James Su, kinuko+fileapi, davemoore+watch_chromium.org, qsr+mojo_chromium.org, tzik, posciak+watch_chromium.org, markusheintz_, stevenjb+watch_chromium.org, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, jdduke+watch_chromium.org, jbauman+watch_chromium.org, chromoting-reviews_chromium.org, hubbe+watch_chromium.org, dbeam+watch-options_chromium.org, cc-bugs_chromium.org, feature-media-reviews_chromium.org, gcasto+watchlist_chromium.org, jsbell+idb_chromium.org, danakj+watch_chromium.org, rouslan+autofillwatch_chromium.org, zork+watch_chromium.org, michaeln, Ian Vollick, mcasas+watch_chromium.org, pwestin+watch_google.com, kinuko+serviceworker, mkwst+moarreviews-renderer_chromium.org, darin (slow to review), scheduler-bugs_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Redirect the MessageLoop's task runner to the renderer scheduler Configure MessageLoop::task_runner() to post tasks through the scheduler to ensure consistent task ordering. BUG=465354 Committed: https://crrev.com/ffca98fdc0f13b60f5e530d5fade7b68f609efd6 Cr-Commit-Position: refs/heads/master@{#339052}

Patch Set 1 #

Patch Set 2 : Defer MessageLoopProxy refactoring for now. #

Patch Set 3 : Cleanup. #

Patch Set 4 : Tests. #

Patch Set 5 : Expanded test. #

Patch Set 6 : No more subclassing. #

Patch Set 7 : Reimplemented against new APIs. Will need to move scheduler construction earlier with another patch. #

Patch Set 8 : Another API update. #

Total comments: 6

Patch Set 9 : Review comments + refactor all the things #

Total comments: 2

Patch Set 10 : Fix test hang by restoring the original task runner on shutdown. #

Patch Set 11 : Fix ScriptContextSetTest.Lifecycle. #

Patch Set 12 : Don't shutdown the scheduler on the worker thread -- just restore the task runner. #

Patch Set 13 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+425 lines, -206 lines) Patch
M components/components_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M components/scheduler/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M components/scheduler/child/idle_helper_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +9 lines, -11 lines 0 comments Download
M components/scheduler/child/nestable_task_runner_for_test.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -2 lines 0 comments Download
M components/scheduler/child/nestable_task_runner_for_test.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -5 lines 0 comments Download
M components/scheduler/child/scheduler_helper.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +7 lines, -7 lines 0 comments Download
M components/scheduler/child/scheduler_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +9 lines, -3 lines 0 comments Download
M components/scheduler/child/scheduler_helper_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +12 lines, -6 lines 0 comments Download
M components/scheduler/child/scheduler_message_loop_delegate.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -48 lines 0 comments Download
M components/scheduler/child/scheduler_message_loop_delegate.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -56 lines 0 comments Download
A components/scheduler/child/scheduler_task_runner_delegate.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +37 lines, -0 lines 0 comments Download
A components/scheduler/child/scheduler_task_runner_delegate_for_test.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +55 lines, -0 lines 0 comments Download
A components/scheduler/child/scheduler_task_runner_delegate_for_test.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +68 lines, -0 lines 0 comments Download
A + components/scheduler/child/scheduler_task_runner_delegate_impl.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +15 lines, -11 lines 0 comments Download
A components/scheduler/child/scheduler_task_runner_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +67 lines, -0 lines 0 comments Download
A components/scheduler/child/scheduler_task_runner_delegate_impl_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +28 lines, -0 lines 0 comments Download
M components/scheduler/child/task_queue_manager_perftest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +3 lines, -2 lines 0 comments Download
M components/scheduler/child/task_queue_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +10 lines, -6 lines 0 comments Download
M components/scheduler/child/webthread_impl_for_worker_scheduler.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -0 lines 0 comments Download
M components/scheduler/child/webthread_impl_for_worker_scheduler.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +19 lines, -3 lines 0 comments Download
M components/scheduler/child/worker_scheduler.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -1 line 0 comments Download
M components/scheduler/child/worker_scheduler.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -4 lines 0 comments Download
M components/scheduler/child/worker_scheduler_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -2 lines 0 comments Download
M components/scheduler/child/worker_scheduler_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -2 lines 0 comments Download
M components/scheduler/child/worker_scheduler_impl_unittest.cc View 1 2 3 4 5 6 7 8 4 chunks +7 lines, -8 lines 0 comments Download
M components/scheduler/renderer/renderer_scheduler.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M components/scheduler/renderer/renderer_scheduler_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M components/scheduler/renderer/renderer_scheduler_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -2 lines 0 comments Download
M components/scheduler/renderer/renderer_scheduler_impl_unittest.cc View 1 2 3 4 5 6 7 8 5 chunks +13 lines, -13 lines 0 comments Download
M components/scheduler/renderer/webthread_impl_for_renderer_scheduler_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -2 lines 0 comments Download
M components/scheduler/scheduler.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +10 lines, -3 lines 0 comments Download
M components/scheduler/test/lazy_scheduler_message_loop_delegate_for_tests.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +25 lines, -4 lines 0 comments Download

Messages

Total messages: 28 (9 generated)
Sami
5 years, 5 months ago (2015-07-07 14:53:35 UTC) #3
alex clarke (OOO till 29th)
https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager.cc File components/scheduler/child/task_queue_manager.cc (right): https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager.cc#newcode515 components/scheduler/child/task_queue_manager.cc:515: main_task_runner->SetDefaultTaskRunner(Queue(0)); It's currently true that task #0 is the ...
5 years, 5 months ago (2015-07-08 13:30:54 UTC) #4
Sami
Thanks Alex. Another look? https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager.cc File components/scheduler/child/task_queue_manager.cc (right): https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager.cc#newcode515 components/scheduler/child/task_queue_manager.cc:515: main_task_runner->SetDefaultTaskRunner(Queue(0)); On 2015/07/08 13:30:53, alexclarke1 ...
5 years, 5 months ago (2015-07-08 18:45:08 UTC) #5
alex clarke (OOO till 29th)
https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager_unittest.cc File components/scheduler/child/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager_unittest.cc#newcode1253 components/scheduler/child/task_queue_manager_unittest.cc:1253: On 2015/07/08 18:45:07, Sami wrote: > On 2015/07/08 13:30:53, ...
5 years, 5 months ago (2015-07-10 15:21:12 UTC) #6
Sami
https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager_unittest.cc File components/scheduler/child/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/987193002/diff/160001/components/scheduler/child/task_queue_manager_unittest.cc#newcode1253 components/scheduler/child/task_queue_manager_unittest.cc:1253: On 2015/07/10 15:21:11, alexclarke1 wrote: > On 2015/07/08 18:45:07, ...
5 years, 5 months ago (2015-07-10 15:35:13 UTC) #7
alex clarke (OOO till 29th)
lgtm
5 years, 5 months ago (2015-07-10 15:56:53 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987193002/220001
5 years, 5 months ago (2015-07-14 16:52:25 UTC) #11
commit-bot: I haz the power
Committed patchset #11 (id:220001)
5 years, 5 months ago (2015-07-14 17:19:18 UTC) #12
commit-bot: I haz the power
Patchset 11 (id:??) landed as https://crrev.com/f14dd762c01730a81d4a94b64ceba42eaad2d5f2 Cr-Commit-Position: refs/heads/master@{#338706}
5 years, 5 months ago (2015-07-14 17:20:02 UTC) #13
Raymond Toy
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/1238483003/ by rtoy@chromium.org. ...
5 years, 5 months ago (2015-07-14 21:11:07 UTC) #14
Sami
I've fixed the test flakiness by avoiding shutting down the scheduler when the WebThread is ...
5 years, 5 months ago (2015-07-16 14:20:37 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987193002/240001
5 years, 5 months ago (2015-07-16 14:21:25 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/86774)
5 years, 5 months ago (2015-07-16 14:49:47 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/987193002/260001
5 years, 5 months ago (2015-07-16 15:29:27 UTC) #23
commit-bot: I haz the power
Committed patchset #13 (id:260001)
5 years, 5 months ago (2015-07-16 16:44:49 UTC) #24
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/ffca98fdc0f13b60f5e530d5fade7b68f609efd6 Cr-Commit-Position: refs/heads/master@{#339052}
5 years, 5 months ago (2015-07-16 16:45:44 UTC) #25
tasak
On 2015/07/16 16:45:44, commit-bot: I haz the power wrote: > Patchset 13 (id:??) landed as ...
5 years, 5 months ago (2015-07-17 02:04:21 UTC) #26
Sami
On 2015/07/17 02:04:21, tasak wrote: > On 2015/07/16 16:45:44, commit-bot: I haz the power wrote: ...
5 years, 5 months ago (2015-07-17 09:54:16 UTC) #27
Sami
5 years, 5 months ago (2015-07-17 10:28:15 UTC) #28
Message was sent while issue was closed.
On 2015/07/17 09:54:16, Sami wrote:
> On 2015/07/17 02:04:21, tasak wrote:
> > On 2015/07/16 16:45:44, commit-bot: I haz the power wrote:
> > > Patchset 13 (id:??) landed as
> > > https://crrev.com/ffca98fdc0f13b60f5e530d5fade7b68f609efd6
> > > Cr-Commit-Position: refs/heads/master@{#339052}
> > 
> > Would you take a look at
> >
>
unexpected_failures:http/tests/webfont/popup-menu-load-webfont-after-open.html?
> > 
> > c.f.
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.9%20%28r...
> > 
> > I'm not sure whether this patch causes the failure or not... however,
looking
> at
> > the following diff:
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_9__...
> > 
> > two "TEST COMPELETE"s were shown. So, I guess, some task-related thing
caused
> > the failure.
> 
> Sure, I'll have a look. On first blush the test looks a little suspicious
since
> it has two calls to finishJsTest() (which prints TEST COMPLETE). The first is
> triggered when the popup fails to open (as expected?) and the second one runs
> when font loading completes. This seems like a race to me but I'll investigate
> more.

See https://code.google.com/p/chromium/issues/detail?id=511021 for more updates.

Powered by Google App Engine
This is Rietveld 408576698