Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(52)

Issue 956333002: Refactor TimeBase to post tasks. Workers to use real Idle tasks. (Closed)

Created:
4 years, 6 months ago by alex clarke (OOO till 29th)
Modified:
4 years, 3 months ago
Reviewers:
Sami, rmcilroy
CC:
rmcilroy_chromium.org blink-reviews, kinuko+worker_chromium.org, horo+watch_chromium.org, falken, picksi1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

This patch refactors TimerBase to post tasks, and deletes the obsolete Blink TimerHeap. Unfortunately this meant refactoring WorkerThreads too (ideally that would have been done in a separate patch). I have change them to use the new WorkerScheduler's idle tasks for v8 GC. ATTN Sheriffs: If there are weird layout test flakes all of a sudden, this patch may be the cause since the interleaving of blink timers with other chromium tasks will change. BUG=463143, 416362, 480522

Patch Set 1 #

Patch Set 2 : Fix bug with CancellableTask destruction after blink heap has gone away #

Patch Set 3 : Fix unit test #

Patch Set 4 : Fix a bunch of stuff #

Total comments: 1

Patch Set 5 : New timer task #

Patch Set 6 : Delete even more! #

Patch Set 7 : Changed the idle implementation #

Total comments: 5

Patch Set 8 : Rebased #

Total comments: 30

Patch Set 9 : Simplify now the quiescence is handled in chromium #

Total comments: 2

Patch Set 10 : Ross's long GC deadlines #

Patch Set 11 : Now uses the WebThread's WebScheduler #

Total comments: 14

Patch Set 12 : Unit tests for TimerBase!!! #

Patch Set 13 : Make WorkerThreadTest thread safe #

Patch Set 14 : Rebase #

Total comments: 17

Patch Set 15 : Some of Ross's suggestions #

Total comments: 10

Patch Set 16 : Fix compilation of TimerTest.cpp on gcc #

Patch Set 17 : Fix componant build #

Patch Set 18 : Ross's comments #

Patch Set 19 : Test using rAF #

Patch Set 20 : Fix ScrollableAreaTest on mac #

Patch Set 21 : Make shutdown of workers safe #

Patch Set 22 : Fix some UAF and other fun bugs #

Patch Set 23 : Fixed several deadlocks. #

Patch Set 24 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1505 lines, -896 lines) Patch
M LayoutTests/fast/dom/HTMLImageElement/image-innerHTML.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +24 lines, -7 lines 0 comments Download
M LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html View 1 2 3 4 5 6 7 8 9 12 13 1 chunk +8 lines, -1 line 0 comments Download
M LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-window-stop.html View 1 2 3 4 5 6 7 8 9 12 13 1 chunk +8 lines, -1 line 0 comments Download
M LayoutTests/http/tests/security/suborigins/resources/childsuborigin.php View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/Init.cpp View 1 2 3 4 5 6 7 12 13 2 chunks +0 lines, -5 lines 0 comments Download
M Source/core/animation/CompositorAnimationsTestHelper.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -1 line 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/fetch/CachingCorrectnessTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -1 line 0 comments Download
M Source/core/fetch/ResourceTest.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -1 line 0 comments Download
M Source/core/frame/DOMTimerCoordinator.cpp View 1 2 3 12 13 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/workers/WorkerGlobalScope.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/workers/WorkerLoaderProxy.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/workers/WorkerThread.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 5 chunks +26 lines, -4 lines 0 comments Download
M Source/core/workers/WorkerThread.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 14 chunks +108 lines, -135 lines 0 comments Download
A Source/core/workers/WorkerThreadTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +457 lines, -0 lines 0 comments Download
D Source/platform/PlatformThreadData.h View 12 13 1 chunk +0 lines, -59 lines 0 comments Download
D Source/platform/PlatformThreadData.cpp View 12 13 1 chunk +0 lines, -63 lines 0 comments Download
D Source/platform/SharedTimer.cpp View 1 2 3 4 5 12 13 1 chunk +0 lines, -48 lines 0 comments Download
D Source/platform/ThreadTimers.h View 1 2 3 4 5 6 7 8 9 10 12 13 1 chunk +0 lines, -67 lines 0 comments Download
D Source/platform/ThreadTimers.cpp View 1 2 3 4 5 6 7 8 9 10 12 13 1 chunk +0 lines, -151 lines 0 comments Download
M Source/platform/Timer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +26 lines, -20 lines 0 comments Download
M Source/platform/Timer.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 6 chunks +46 lines, -309 lines 0 comments Download
A Source/platform/TimerTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +680 lines, -0 lines 0 comments Download
M Source/platform/blink_platform.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +1 line, -6 lines 0 comments Download
M Source/platform/blink_platform_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -0 lines 0 comments Download
M Source/platform/scroll/ScrollableAreaTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +78 lines, -0 lines 0 comments Download
M Source/web/tests/TextFinderTest.cpp View 1 2 3 4 5 6 12 13 1 chunk +0 lines, -5 lines 0 comments Download
M Source/wtf/CurrentTime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M public/platform/Platform.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -6 lines 0 comments Download
M public/platform/WebScheduler.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +10 lines, -2 lines 0 comments Download

Messages

Total messages: 41 (15 generated)
alex clarke (OOO till 29th)
4 years, 6 months ago (2015-02-27 17:44:00 UTC) #1
alex clarke (OOO till 29th)
4 years, 6 months ago (2015-02-27 17:44:01 UTC) #2
Sami
https://codereview.chromium.org/956333002/diff/60001/Source/core/dom/ScriptRunnerTest.cpp File Source/core/dom/ScriptRunnerTest.cpp (right): https://codereview.chromium.org/956333002/diff/60001/Source/core/dom/ScriptRunnerTest.cpp#newcode90 Source/core/dom/ScriptRunnerTest.cpp:90: Scheduler::shutdown(); I wonder if we should have some testing ...
4 years, 6 months ago (2015-02-27 18:29:52 UTC) #4
Sami
If there's any way to split out killing the timer heap from implementing idle work ...
4 years, 5 months ago (2015-04-02 10:59:20 UTC) #7
alex clarke (OOO till 29th)
I'm not entirely sure how to land the Worker idle changes separately (they'd have to ...
4 years, 5 months ago (2015-04-08 15:43:35 UTC) #8
Sami
Fair enough if there's no easy way to split this up. I've added one suggestion ...
4 years, 5 months ago (2015-04-09 10:52:30 UTC) #9
alex clarke (OOO till 29th)
Many of the comment no longer apply after moving the quiescence check to chromium. https://codereview.chromium.org/956333002/diff/200001/Source/core/dom/ScriptRunnerTest.cpp ...
4 years, 5 months ago (2015-04-10 15:29:42 UTC) #10
rmcilroy
https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (left): https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/WorkerThread.cpp#oldcode515 Source/core/workers/WorkerThread.cpp:515: int64_t delay = kLongIdleHandlerDelayMs; I've just realized that this ...
4 years, 5 months ago (2015-04-13 13:55:07 UTC) #11
alex clarke (OOO till 29th)
https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (left): https://codereview.chromium.org/956333002/diff/220001/Source/core/workers/WorkerThread.cpp#oldcode515 Source/core/workers/WorkerThread.cpp:515: int64_t delay = kLongIdleHandlerDelayMs; On 2015/04/13 13:55:07, rmcilroy wrote: ...
4 years, 5 months ago (2015-04-14 13:04:35 UTC) #12
alex clarke (OOO till 29th)
PTAL
4 years, 5 months ago (2015-04-15 15:15:39 UTC) #13
Sami
Some minor comments. https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/WorkerLoaderProxy.h File Source/core/workers/WorkerLoaderProxy.h (right): https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/WorkerLoaderProxy.h#newcode88 Source/core/workers/WorkerLoaderProxy.h:88: protected: Could you add a comment ...
4 years, 5 months ago (2015-04-16 14:23:39 UTC) #14
alex clarke (OOO till 29th)
PTAL https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/WorkerLoaderProxy.h File Source/core/workers/WorkerLoaderProxy.h (right): https://codereview.chromium.org/956333002/diff/260001/Source/core/workers/WorkerLoaderProxy.h#newcode88 Source/core/workers/WorkerLoaderProxy.h:88: protected: On 2015/04/16 14:23:38, Sami wrote: > Could ...
4 years, 5 months ago (2015-04-16 17:20:01 UTC) #15
rmcilroy
Loving all the red :). A couple of comments but looks good overall for the ...
4 years, 5 months ago (2015-04-17 12:59:33 UTC) #16
alex clarke (OOO till 29th)
https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html File LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html (right): https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html#newcode7 LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:7: window.onload = function() { On 2015/04/17 12:59:32, rmcilroy wrote: ...
4 years, 5 months ago (2015-04-17 13:37:08 UTC) #17
Sami
Thanks Alex, lgtm. I'll let Ross okay the idle gc logic. https://codereview.chromium.org/956333002/diff/340001/LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html File LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html (right): ...
4 years, 5 months ago (2015-04-17 15:34:32 UTC) #18
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956333002/340001
4 years, 5 months ago (2015-04-17 16:02:32 UTC) #20
rmcilroy
WorkerThread and WorkerThreadTest lgtm. Thanks! https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html File LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html (right): https://codereview.chromium.org/956333002/diff/320001/LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html#newcode7 LayoutTests/fast/events/resources/subframe-stop-load-in-unload-handler-using-document-write.html:7: window.onload = function() { ...
4 years, 5 months ago (2015-04-17 16:33:22 UTC) #21
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/builds/42193)
4 years, 5 months ago (2015-04-17 16:38:27 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956333002/360001
4 years, 5 months ago (2015-04-17 16:48:10 UTC) #26
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/builds/42198)
4 years, 5 months ago (2015-04-17 17:32:45 UTC) #28
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956333002/380001
4 years, 5 months ago (2015-04-20 09:49:22 UTC) #31
alex clarke (OOO till 29th)
https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/WorkerThread.cpp#newcode467 Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds)) On 2015/04/17 16:33:22, rmcilroy wrote: > On ...
4 years, 5 months ago (2015-04-20 10:06:54 UTC) #32
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956333002/440001
4 years, 5 months ago (2015-04-20 15:57:17 UTC) #35
rmcilroy
https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/WorkerThread.cpp File Source/core/workers/WorkerThread.cpp (right): https://codereview.chromium.org/956333002/diff/320001/Source/core/workers/WorkerThread.cpp#newcode467 Source/core/workers/WorkerThread.cpp:467: if (doIdleGc(gcDeadlineSeconds)) On 2015/04/20 10:06:53, alexclarke1 wrote: > On ...
4 years, 5 months ago (2015-04-20 20:56:50 UTC) #36
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52592)
4 years, 5 months ago (2015-04-20 21:08:30 UTC) #38
Sami
4 years, 4 months ago (2015-05-07 15:57:27 UTC) #41

Powered by Google App Engine
This is Rietveld 408576698