Reland toBlob: If idle image encoding is postponed for too long, switch to main thread

Reland toBlob: If idle image encoding is postponed for too long, switch to main thread

The original patch (https://codereview.chromium.org/1665413004/) was reverted 3
months ago due to a problem in WebKit Leak bot. The recent changes in Oilpan have
resolved this problem.

BUG=583070
Committed: https://crrev.com/b1781a606269a2d8866b0d9a14ae67b4c9c33afd
Cr-Commit-Position: refs/heads/master@{#391372}

[xlai (Olivia), 2016-05-02 20:13:35]

xlai@chromium.org changed reviewers:
+ junov@chromium.org

[xlai (Olivia), 2016-05-02 20:13:35]

The idea is same as the old patch that get reverted in February--that is, when
idle task is too slow to happen, we force toBlob to happen on main thread; but
many details are different due to recent changes in Oilpan and Scheduling.

Btw, I checked whether this patch is able to fix user-reported bug
(crbug.com/606796) and below is my observation:

1. Without this patch (master branch)

1a. After clicking "Blobify", no mouse moving at all
Observation: toBlob never finishes

1b. After clicking "Blobify", keep hovering mouse movement
Observation: toBlob finishes within 4 seconds

2. With this patch

2a. After clicking "Blobify", no mouse moving at all
Observation: toBlob finishes within 9 seconds

2b. After clicking "Blobify", keep hovering mouse movement
Observation: toBlob finishes within 3.5 seconds

In conclusion, this patch solves the problem of toBlob getting forever delayed;
but it cannot solve the problem that without mouse movement toBlob happens so
slow. 

I think it is a problem in the idle task mechanism--that is, idle tasks can get
delayed indefinitely if there's no mouse movement at all. Because of that,
toBlob in this patch will 100% likely to get switched to alternative code path
as the original idle task doesn't occur within our timeout delay limit.

[Justin Novosad, 2016-05-03 14:19:19]

This looks great!  Mostly superficial comments.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:32: //
lenient limit -- 200ms here.
Should also explain that the limit still need to be sort enough for the latency
to be negligible to the user.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:41: const
double IdleTaskCompleteTimeoutDelay = 10000.0;
This delay seems a bit excessive. 4k x 4k is a rather atypical load on mobile
IMHO.  But this is just a gut feeling.  Would be a good idea to collect
histogram stats on real-world  timings and frequencies at which the various
timeout delays are hit. Could you file a follow-up crbug for that?

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:65:
m_idleTaskStatus = IdleTaskStatus::Default;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:82:
m_idleTaskStatus = IdleTaskStatus::IdleTaskNotStarted;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:115: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskSwitchedToMainThreadTask) {
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:119:
ASSERT(m_idleTaskStatus == IdleTaskStatus::IdleTaskNotStarted);
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:120:
m_idleTaskStatus = IdleTaskStatus::IdleTaskStarted;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:123:
m_idleTaskStatus = IdleTaskStatus::IdleTaskFailed;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:132: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskSwitchedToMainThreadTask) {
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:149:
m_idleTaskStatus = IdleTaskStatus::IdleTaskCompleted;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:160:
ASSERT(m_idleTaskStatus == IdleTaskStatus::IdleTaskSwitchedToMainThreadTask);
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:181: void
CanvasAsyncBlobCreator::createNullptrAndCall()
Naming Nit: what are we calling?

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:219: //
Even if the task started quickly, we still want to ensure completion
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:220:
this->postDelayedTaskToMainThread(BLINK_FROM_HERE, new
SameThreadTask(bind(&CanvasAsyncBlobCreator::idleTaskCompleteTimeoutEvent,
this)), IdleTaskCompleteTimeoutDelay);
This task should be posted when idle task starts (in initiatePngEncoding). By
grouping action with state transition, the state machine logic is more readable
and easier to reason about.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:221: }
else if (m_idleTaskStatus == IdleTaskStatus::IdleTaskNotStarted) {
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:225:
m_idleTaskStatus = IdleTaskStatus::IdleTaskSwitchedToMainThreadTask;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:234: //
When IdleTaskStatus is either IdleTaskFailed or IdleTaskCompleted
ASSERT this

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:242:
ASSERT(m_idleTaskStatus != IdleTaskStatus::IdleTaskNotStarted);
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:244: if
(m_idleTaskStatus == IdleTaskStatus::IdleTaskStarted) {
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:246:
m_idleTaskStatus = IdleTaskStatus::IdleTaskSwitchedToMainThreadTask;
No need for "IdleTaskStatus::"

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:250: //
When IdleTaskStatus is either IdleTaskFailed or IdleTaskCompleted
Add an ASSERT to verify the status

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:31: Default
// Idle tasks are not implemented for some image types
Use a more meaningful name. Perhaps IdleTaskNotSupported or something like that.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:33: virtual
void signalTaskSwitchInStartTimeoutEventForTesting() { }
Add comment: // Methods are virtual for mocking in unit tests

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:43: virtual
void scheduleInitiatePngEncoding();
Comment: // Methods are virtual for unit testing

[xlai (Olivia), 2016-05-03 18:01:27]

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:41: const
double IdleTaskCompleteTimeoutDelay = 10000.0;
On 2016/05/03 14:19:19, Justin Novosad wrote:
> This delay seems a bit excessive. 4k x 4k is a rather atypical load on mobile
> IMHO.  But this is just a gut feeling.  Would be a good idea to collect
> histogram stats on real-world  timings and frequencies at which the various
> timeout delays are hit. Could you file a follow-up crbug for that?

Acknowledged. Crbug.com/608815 created.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:181: void
CanvasAsyncBlobCreator::createNullptrAndCall()
On 2016/05/03 14:19:19, Justin Novosad wrote:
> Naming Nit: what are we calling?

Changed the two relevant function names to createNullAndInvokeCallback() and
createBlobAndInvokeCallback()

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:220:
this->postDelayedTaskToMainThread(BLINK_FROM_HERE, new
SameThreadTask(bind(&CanvasAsyncBlobCreator::idleTaskCompleteTimeoutEvent,
this)), IdleTaskCompleteTimeoutDelay);
On 2016/05/03 14:19:18, Justin Novosad wrote:
> This task should be posted when idle task starts (in initiatePngEncoding). By
> grouping action with state transition, the state machine logic is more
readable
> and easier to reason about.

junov@: I think putting this postDelayedTask in initiatePngEncoding is
dangerous. It may cause the following unwanted scenarios to occur:

1. idleTaskCompleteTimeoutEvent occurs before idleTaskStartTimeoutEvent
-- It is possible that when scheduleInitiatePngEncoding() is invoked, the idle
task is very quick and immediately start executing and fires
CompleteTimeoutEvent, before we reach the statement when StartTimeoutEvent is
posted. The logic inside these two timeout events would be quite messed out and
need to consider all sorts of possible scenarios. The structure of the unit test
needs to be drastically changed too.

2. When either CompleteTimeoutEvent() or StartTimeoutEvent() (as mentioned in
point 1, there is no definite order between the two events after the change) has
already confirmed that the toBlob idle task has completed or has failed, we
still cannot say that the whole process has finished, because the other timeout
event will still happen no matter the result of the first timeout event. It is a
waste of resources to do double checking.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:234: //
When IdleTaskStatus is either IdleTaskFailed or IdleTaskCompleted
On 2016/05/03 14:19:19, Justin Novosad wrote:
> ASSERT this

Done here and elsewhere.

https://codereview.chromium.org/1938103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:246:
m_idleTaskStatus = IdleTaskStatus::IdleTaskSwitchedToMainThreadTask;
On 2016/05/03 14:19:18, Justin Novosad wrote:
> No need for "IdleTaskStatus::"

Done here and elsewhere.

[Justin Novosad, 2016-05-03 19:53:36]

lgtm

[xlai (Olivia), 2016-05-03 20:03:59]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-05-03 20:04:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1938103002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1938103002/20001

[xlai (Olivia), 2016-05-03 20:13:33]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2016-05-03 20:52:49]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-05-03 20:52:50]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1938103002/#ps40001
(title: "Layout tests leak fixed")

[commit-bot: I haz the power, 2016-05-03 20:53:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1938103002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1938103002/40001

[commit-bot: I haz the power, 2016-05-03 22:07:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-03 22:08:52]

Description was changed from

==========
Reland toBlob: If idle image encoding is postponed for too long, switch to main
thread

The original patch (https://codereview.chromium.org/1665413004/) was reverted 3
months ago due to a problem in WebKit Leak bot. The recent changes in Oilpan
have
resolved this problem.

BUG=583070
==========

to

==========
Reland toBlob: If idle image encoding is postponed for too long, switch to main
thread

The original patch (https://codereview.chromium.org/1665413004/) was reverted 3
months ago due to a problem in WebKit Leak bot. The recent changes in Oilpan
have
resolved this problem.

BUG=583070

Committed: https://crrev.com/b1781a606269a2d8866b0d9a14ae67b4c9c33afd
Cr-Commit-Position: refs/heads/master@{#391372}
==========

[commit-bot: I haz the power, 2016-05-03 22:08:53]

Patchset 3 (id:??) landed as
https://crrev.com/b1781a606269a2d8866b0d9a14ae67b4c9c33afd
Cr-Commit-Position: refs/heads/master@{#391372}