Force events to be non blocking if main thread is unresponsive. (issue 2273703002 by tdresser@chromium.org)

[tdre...@chromium.org]

Reviewers: lanwei, dtapuska
CL: https://codereview.chromium.org/2273703002/

Message:
PTAL

Description:
Force events to be non blocking if main thread is unresponsive.

Behind the MainThreadBusyScrollIntervention content feature.

Uses the estimated input latency
(https://docs.google.com/document/d/1b9slyaB9yho91YTOkAQfpCdULFkZM9LqsipcX3t7He8/)
to determine when the main thread is unresponsive. When the main thread
is unresponsive, forces events to be non-blocking.

To be enabled via Finch.

TEST=MainThreadEventQueueTest.ForcedNonBlockingDueToUnresponsiveMainThread,
RenderWidgetUnittest.RenderWidgetInputEventUmaMetrics

QueueingTimeEstimatorTest.EstimateQueueingTimeDuringSingleLongTaskIncompleteWindow

QueueingTimeEstimatorTest.EstimateQueueingTimeDuringSingleLongTaskExceedingWindow
BUG=599609

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+414, -128 lines):
M content/public/common/content_features.h
M content/public/common/content_features.cc
M content/renderer/input/main_thread_event_queue.h
M content/renderer/input/main_thread_event_queue.cc
M content/renderer/input/main_thread_event_queue_unittest.cc
M content/renderer/input/render_widget_input_handler.cc
M content/renderer/render_widget_unittest.cc
M third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
M third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
M third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc
M third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc
M third_party/WebKit/Source/platform/scheduler/base/task_time_tracker.h
M third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
M third_party/WebKit/Source/platform/scheduler/test/fake_renderer_scheduler.cc
M third_party/WebKit/public/platform/WebInputEvent.h
M third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h
M third_party/WebKit/public/platform/scheduler/test/fake_renderer_scheduler.h
M third_party/WebKit/public/platform/scheduler/test/mock_renderer_scheduler.h
M tools/metrics/histograms/histograms.xml

[dtap...@chromium.org]

https://codereview.chromium.org/2273703002/diff/1/content/public/common/content_features.cc
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/public/common/content_features.cc#newcode71
content/public/common/content_features.cc:71: // passive when the main
thread is deemed unresponsive.
s/passive/non-blocking ?

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue.h
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue.h#newcode140
content/renderer/input/main_thread_event_queue.h:140: bool
enable_non_blocking_due_to_main_thread_responsiveness_flag_;
I think you win for the longest variable name in this class :-)

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode111
content/renderer/input/main_thread_event_queue_unittest.cc:111: const
WebTouchEvent* last_touch_event() {
should be a const function

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode119
content/renderer/input/main_thread_event_queue_unittest.cc:119: const
WebMouseWheelEvent* first_wheel_event() {
ditto

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode127
content/renderer/input/main_thread_event_queue_unittest.cc:127: const
WebMouseWheelEvent* last_wheel_event() {
ditto

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode171
content/renderer/input/main_thread_event_queue_unittest.cc:171:
EXPECT_TRUE(AreEqual(coalesced_event, *first_wheel_event()));
Isn't this a null deref?

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/render_widget_input_handler.cc
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/render_widget_input_handler.cc#newcode132
content/renderer/input/render_widget_input_handler.cc:132: // TODO - add
UMAs.
Is this TODO still necessary?

https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1346
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1346:
base::TimeTicks now = base::TimeTicks::Now();
How does this code deal with non-high precision clocks?

ie; what is going to happen on those machines with high latency time.

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

https://codereview.chromium.org/2273703002/diff/1/content/public/common/content_features.cc
File content/public/common/content_features.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/public/common/content_features.cc#newcode71
content/public/common/content_features.cc:71: // passive when the main
thread is deemed unresponsive.

On 2016/08/23 16:18:29, dtapuska wrote:
> s/passive/non-blocking ?

Done.

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue.h
File content/renderer/input/main_thread_event_queue.h (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue.h#newcode140
content/renderer/input/main_thread_event_queue.h:140: bool
enable_non_blocking_due_to_main_thread_responsiveness_flag_;

On 2016/08/23 16:18:29, dtapuska wrote:
> I think you win for the longest variable name in this class :-)

I'm open to alternative suggestions!

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc
File content/renderer/input/main_thread_event_queue_unittest.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode111
content/renderer/input/main_thread_event_queue_unittest.cc:111: const
WebTouchEvent* last_touch_event() {

On 2016/08/23 16:18:29, dtapuska wrote:
> should be a const function

Done.

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode119
content/renderer/input/main_thread_event_queue_unittest.cc:119: const
WebMouseWheelEvent* first_wheel_event() {

On 2016/08/23 16:18:29, dtapuska wrote:
> ditto

Done.

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode127
content/renderer/input/main_thread_event_queue_unittest.cc:127: const
WebMouseWheelEvent* last_wheel_event() {

On 2016/08/23 16:18:29, dtapuska wrote:
> ditto

Done.

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main_thread_event_queue_unittest.cc#newcode171
content/renderer/input/main_thread_event_queue_unittest.cc:171:
EXPECT_TRUE(AreEqual(coalesced_event, *first_wheel_event()));

On 2016/08/23 16:18:29, dtapuska wrote:
> Isn't this a null deref?

Added CHECK to ensure that these methods never return nullptr.

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/render_widget_input_handler.cc
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/render_widget_input_handler.cc#newcode132
content/renderer/input/render_widget_input_handler.cc:132: // TODO - add
UMAs.

On 2016/08/23 16:18:29, dtapuska wrote:
> Is this TODO still necessary?

Done.

https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1346
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1346:
base::TimeTicks now = base::TimeTicks::Now();

On 2016/08/23 16:18:29, dtapuska wrote:
> How does this code deal with non-high precision clocks?
>
> ie; what is going to happen on those machines with high latency time.

What's your concern, that we'll trigger too aggressively?

We're currently on activating if the expected queueing time is greater
than a full second, which is generous enough that I don't think there's
any risk there.

https://codereview.chromium.org/2273703002/

[dtap...@chromium.org]

lgtm



https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

+skyostil for scheduling logic.

https://codereview.chromium.org/2273703002/

[lan...@chromium.org]

LGTM!

https://codereview.chromium.org/2273703002/

[skyostil@chromium.org via codereview.chromium.org]

Thanks Tim, added some comments.


https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
File
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc#newcode98
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:98:
base::TimeDelta worst_queueing_time() {
naming nit: "worst_queueing_time" is subjective but "max_queueing_time"
would be objective and match the name of this class :)

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
File
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h#newcode46
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:46:
std::unique_ptr<QueueingTimeEstimator> CloneWithClient(Client* client)
const;
Is it necessary for this to be a heap allocation? It seems like we could
make this a private copy constructor and keep the temporary class on the
stack.

Alternatively we could split the computation backend from the client
reporting part and only copy the former, but I'm not sure if it would be
worth it.

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h
File
third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h#newcode16
third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h:16:
void ReportTaskStartTime(base::TimeTicks startTime) override {}
nit: This feels weirdly asymmetric now that both methods take
|startTime|. Maybe we only pass it to the former?

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1431
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1431:
base::AutoLock lock(any_thread_lock_);
This makes us take two locks for every task we execute which seems high
to me (even if they should generally be uncontented). I wonder if
there's a way to avoid that? How stale an estimate would still work?

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
File
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc#newcode98
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:98:
base::TimeDelta worst_queueing_time() {

On 2016/08/24 14:56:01, Sami wrote:
> naming nit: "worst_queueing_time" is subjective but
"max_queueing_time" would be
> objective and match the name of this class :)

Done.

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
File
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h#newcode46
third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:46:
std::unique_ptr<QueueingTimeEstimator> CloneWithClient(Client* client)
const;

On 2016/08/24 14:56:01, Sami wrote:
> Is it necessary for this to be a heap allocation? It seems like we
could make
> this a private copy constructor and keep the temporary class on the
stack.
>
> Alternatively we could split the computation backend from the client
reporting
> part and only copy the former, but I'm not sure if it would be worth
it.

Done.

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h
File
third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h#newcode16
third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h:16:
void ReportTaskStartTime(base::TimeTicks startTime) override {}

On 2016/08/24 14:56:01, Sami wrote:
> nit: This feels weirdly asymmetric now that both methods take
|startTime|. Maybe
> we only pass it to the former?

Done.

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1431
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1431:
base::AutoLock lock(any_thread_lock_);

On 2016/08/24 14:56:01, Sami wrote:
> This makes us take two locks for every task we execute which seems
high to me
> (even if they should generally be uncontented). I wonder if there's a
way to
> avoid that? How stale an estimate would still work?

I think there are basically two options here:

The problem case is when the main thread is running an extremely long
task, and then partway through that task, the compositor thread needs to
know the estimated queueing time. This requires knowing the start time
of that extremely long task.

Either we lock, and ask when the task started, or we get the main thread
to post over to the compositor when each task started.

An estimate whose staleness is bounded in milliseconds would be fine. An
estimate whose staleness is bounded in number of main thread tasks is
not.

I should be able to get us down to a single lock per task, instead of
two, if we think that's adequate.

Do you have any other ideas?

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

Note: In rebasing, I'm ending up undoing the long_task_tracker changes.

https://codereview.chromium.org/2273703002/

[skyostil@chromium.org via codereview.chromium.org]

Thanks Tim.

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1431
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1431:
base::AutoLock lock(any_thread_lock_);

On 2016/08/24 17:22:50, tdresser wrote:
> On 2016/08/24 14:56:01, Sami wrote:
> > This makes us take two locks for every task we execute which seems
high to me
> > (even if they should generally be uncontented). I wonder if there's
a way to
> > avoid that? How stale an estimate would still work?
>
> I think there are basically two options here:
>
> The problem case is when the main thread is running an extremely long
task, and
> then partway through that task, the compositor thread needs to know
the
> estimated queueing time. This requires knowing the start time of that
extremely
> long task.

Do you think this happens often enough that we actually need to handle
this case? It feels like hitting a long task without also seeing other
long tasks on the page prior to that is pretty rare, and getting a jank
in that case is probably fine.

> Either we lock, and ask when the task started, or we get the main
thread to post
> over to the compositor when each task started.
>
> An estimate whose staleness is bounded in milliseconds would be fine.
An
> estimate whose staleness is bounded in number of main thread tasks is
not.
>
> I should be able to get us down to a single lock per task, instead of
two, if we
> think that's adequate.
>
> Do you have any other ideas?

Another thought I had is to do an atomic store to a variable that is
sampled by the compositor thread. Much cheaper than a lock, but still
not entirely free.

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
(right):

https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1431
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1431:
base::AutoLock lock(any_thread_lock_);

One of the biggest cases we want to address with this intervention is
the very first scroll on a page. I think this does happen often enough
that we need to address it.

We could do an atomic store of the task start time.

What I really want here is an atomic base::TimeTicks – I'm not sure what
atomics look like in chromium, can you point me in the right direction?

https://codereview.chromium.org/2273703002/

[skyostil@chromium.org via codereview.chromium.org]

On 2016/08/25 15:27:12, tdresser wrote:
> One of the biggest cases we want to address with this intervention is the very
> first scroll on a page. I think this does happen often enough that we need to
> address it.

Sure, that's an important use case. I was just trying to think of ways to
address it that don't involve adding overhead to all tasks in the system. Maybe
some model where we presume the main thread is unresponsive until proven
otherwise...?

> We could do an atomic store of the task start time.
>
> What I really want here is an atomic base::TimeTicks – I'm not sure what
atomics
> look like in chromium, can you point me in the right direction?

Look in https://cs.chromium.org/chromium/src/base/atomicops.h

I think we want a NoBarrier_Store on the main thread and a NoBarrier_Load on the
compositor thread, because we aren't reading any data that depends on the value
the atomic read returns.

Unfortunately only 32 bit atomics are generally available, so we might have to
poll something that fits into that space.

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

How does the cost of an atomic write compare to the cost of getting a lock?
Contention is extremely unlikely, a spin lock is just as fast as an atomic
right, isn't it?

We could do something like post a QueueingTimeEstimator from the main thread to
the compositor thread every N ms, and then if we don't get that message, assume
the main thread is busy. That's going to have a bunch of overhead too though,
and we'll have to do some bookkeeping to keep track of when we go idle, so we
don't wake up the CPU needlessly.

https://codereview.chromium.org/2273703002/

[skyostil@chromium.org via codereview.chromium.org]

On 2016/08/25 21:02:36, tdresser wrote:
> How does the cost of an atomic write compare to the cost of getting a lock?
> Contention is extremely unlikely, a spin lock is just as fast as an atomic
> right, isn't it?

I don't think we're using spinlocks at the moment. pthread's mutex is an actual
mutex. I suspect an atomic write is still faster. The task posting
microbenchmark could give us some indication.

We've tried hard to avoid locks as much as possible in the scheduler
(base::MessageLoop does that too) so I'd like us to be really careful about
adding new ones -- contented or not :)

> We could do something like post a QueueingTimeEstimator from the main thread
to
> the compositor thread every N ms, and then if we don't get that message,
assume
> the main thread is busy. That's going to have a bunch of overhead too though,
> and we'll have to do some bookkeeping to keep track of when we go idle, so we
> don't wake up the CPU needlessly.

Yeah, that might be tricky to do without causing extra work. Every PostTask is
also likely to deschedule the calling thread, esp. if the target thread has
higher priority.

https://codereview.chromium.org/2273703002/

[tdre...@chromium.org]

Jotted down some options here:
https://docs.google.com/document/d/1ZcImL7VkZKKXpefRE7VCd5VvWOpqi96Oda_om-Q7OGU/edit?usp=sharing

https://codereview.chromium.org/2273703002/