Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273703002:
Force events to be non blocking if main thread is unresponsive. (Closed)
|
Created:
4 years, 4 months ago by tdresser Modified:
4 years ago Reviewers:
Ken Rockot(use gerrit already), jochen (gone - plz use gerrit), Sami, dtapuska, jbroman, lanwei, Ilya Sherman, alex clarke (OOO till 29th) CC:
altimin, asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dtapuska+chromiumwatch_chromium.org, jam, mlamouri+watch-content_chromium.org, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce 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
Committed: https://crrev.com/2c6c355f995d54c481419acebed7f988207c57c4
Cr-Commit-Position: refs/heads/master@{#439106}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address Dave's comments. #
Total comments: 10
Patch Set 3 : Address Sami's feedback, except for frequent locking. #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #Patch Set 6 : Use SeqLock - needs some additional tests. #Patch Set 7 : Fix build. #
Total comments: 9
Patch Set 8 : Address feedback. #
Total comments: 7
Patch Set 9 : Address feedback (still needs tests) #Patch Set 10 : Rebase. #Patch Set 11 : Add tests, fix bugs. #
Total comments: 24
Patch Set 12 : Rebase. #Patch Set 13 : Address nits. #
Total comments: 4
Patch Set 14 : Fix nits #
Total comments: 4
Patch Set 15 : Nits #
Total comments: 2
Patch Set 16 : Renamed ReadOrFail to TryRead. #
Total comments: 4
Patch Set 17 : Histogram nits #Messages
Total messages: 91 (50 generated)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tdresser@chromium.org changed reviewers: + dtapuska@chromium.org, lanwei@chromium.org
PTAL
https://codereview.chromium.org/2273703002/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/public/common/conte... 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... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main... 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... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main... 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... 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... 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... 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/rend... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/rend... 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/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/p... 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2273703002/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/public/common/conte... 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... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main... 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... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/main... 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... 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... 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... 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/rend... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/2273703002/diff/1/content/renderer/input/rend... 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/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/1/third_party/WebKit/Source/p... 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.
lgtm
tdresser@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil for scheduling logic.
LGTM!
Thanks Tim, added some comments. https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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?
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2016/08/24 17:22:50, tdresser wrote: > https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc > (right): > > https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... > 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/Sour... > File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h > (right): > > https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... > 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/Sour... > File third_party/WebKit/Source/platform/scheduler/base/test_task_time_tracker.h > (right): > > https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... > 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/Sour... > File > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc > (right): > > https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... > 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? Note: In rebasing, I'm ending up undoing the long_task_tracker changes.
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks Tim. https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... 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/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1431: base::AutoLock lock(any_thread_lock_); On 2016/08/25 12:48:44, Sami wrote: > 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. 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?
Description was changed from ========== 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 ========== to ========== 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 ==========
skyostil@chromium.org changed reviewers: + alexclarke@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.
On 2016/08/25 16:12:53, Sami wrote: > 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. 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.
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.
On 2016/08/26 10:39:33, Sami wrote: > 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. Jotted down some options here: https://docs.google.com/document/d/1ZcImL7VkZKKXpefRE7VCd5VvWOpqi96Oda_om-Q7O...
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've migrated over to a SeqLock, which just assumes the main thread isn't busy if there's contention. It seems pretty reasonable, but I'm not certain what the best way to test the cases with contention is. I could maybe inject a subclass of the QueueingTimeEstimator for which writes are super slow - maybe they actually wait on some signal to complete? Does that sound reasonable? How does this approach look? In particular, the logic with the SeqLock in renderer_scheduler_impl.cc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks Tim! https://codereview.chromium.org/2273703002/diff/120001/device/base/synchroniz... File device/base/synchronization/shared_memory_seqlock_buffer.h (right): https://codereview.chromium.org/2273703002/diff/120001/device/base/synchroniz... device/base/synchronization/shared_memory_seqlock_buffer.h:24: SharedMemorySeqLockBuffer(Data data) : data(data) {} nit: explicit https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:46: const base::TimeDelta kMainThreadUnresponsiveQueueingTime = constexpr to avoid a global static initializer. https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1458: // TODO - is this copy okay? Use memcpy? I'd need to think about this a bit more, but I think this is only safe for POD data. If there are pointers or objects in the buffer, we might read their values correctly but the objects they are pointing to (which live outside the buffer) aren't safe to access. Would it be easy to just write numbers into the buffer? https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:189: virtual bool ShouldForceEventsNonBlockingForUnresponsiveMainThread() This probably deserves a doc comment here. I'd also suggest renaming this to something like MainThreadSeemsUnresponsive() since we could potentially make other decisions based on this too and the code in the scheduler doesn't really care about non-blocking events per se.
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. Any idea on the best way to test the cases with contention? https://codereview.chromium.org/2273703002/diff/120001/device/base/synchroniz... File device/base/synchronization/shared_memory_seqlock_buffer.h (right): https://codereview.chromium.org/2273703002/diff/120001/device/base/synchroniz... device/base/synchronization/shared_memory_seqlock_buffer.h:24: SharedMemorySeqLockBuffer(Data data) : data(data) {} On 2016/11/02 19:01:42, Sami wrote: > nit: explicit Done. https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:46: const base::TimeDelta kMainThreadUnresponsiveQueueingTime = On 2016/11/02 19:01:42, Sami wrote: > constexpr to avoid a global static initializer. Done. https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1458: // TODO - is this copy okay? Use memcpy? On 2016/11/02 19:01:42, Sami wrote: > I'd need to think about this a bit more, but I think this is only safe for POD > data. If there are pointers or objects in the buffer, we might read their values > correctly but the objects they are pointing to (which live outside the buffer) > aren't safe to access. Would it be easy to just write numbers into the buffer? Done, though it's a bit ugly. This is probably a bit conservative (I suspect we could safely copy base::TimeTicks/base::TimeDelta, but this is the clearest approach, and should make the critical sections as short as possible). https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:189: virtual bool ShouldForceEventsNonBlockingForUnresponsiveMainThread() On 2016/11/02 19:01:42, Sami wrote: > This probably deserves a doc comment here. I'd also suggest renaming this to > something like MainThreadSeemsUnresponsive() since we could potentially make > other decisions based on this too and the code in the scheduler doesn't really > care about non-blocking events per se. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1458: // TODO - is this copy okay? Use memcpy? On 2016/11/03 13:56:10, tdresser wrote: > On 2016/11/02 19:01:42, Sami wrote: > > I'd need to think about this a bit more, but I think this is only safe for POD > > data. If there are pointers or objects in the buffer, we might read their > values > > correctly but the objects they are pointing to (which live outside the buffer) > > aren't safe to access. Would it be easy to just write numbers into the buffer? > > Done, though it's a bit ugly. This is probably a bit conservative (I suspect we > could safely copy base::TimeTicks/base::TimeDelta, but this is the clearest > approach, and should make the critical sections as short as possible). I'm not sure what we gain by /not/ copying the base::TimeTicks/base::TimeDelta.
> Any idea on the best way to test the cases with contention? Might be tricky -- maybe the best way is to add some test hooks that let you simulate contention. https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:37: using SerializedQueueingTimeEstimator = std::array<int64_t, 4>; Could this be a normal struct instead? That way we could have names for the items and it would generally be more obvious what's going on. We could also make it a member of the live class to reduce duplication. (TimeTicks/Delta don't have vtables or pointers so they are safe to pass through the shared buffer.) https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1459: return false; Could we instead cache the previous result and return that here? I think it might be a bit surprising to randomly switch our answer depending on if we got lucky with the critical section or not. https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1576: seqlock_queueing_time_estimator_.seqlock.WriteBegin(); Is there a way to do this write less often, e.g., only when the answer changes?
https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1459: return false; On 2016/11/03 16:43:15, Sami wrote: > Could we instead cache the previous result and return that here? I think it > might be a bit surprising to randomly switch our answer depending on if we got > lucky with the critical section or not. I'd prefer we err on the side of not intervening, but I don't feel strongly about it. In practice, I don't think we'll run into this often at all anyways (especially when there are long tasks). It doesn't seem worth the extra (small amount of) complexity. Do you think this might actually matter? https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1576: seqlock_queueing_time_estimator_.seqlock.WriteBegin(); On 2016/11/03 16:43:15, Sami wrote: > Is there a way to do this write less often, e.g., only when the answer changes? What do you mean by "the answer"? The most recent task end time will always have changed, and we require that knowledge for computing the EQT.
https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1459: return false; On 2016/11/03 18:23:54, tdresser wrote: > On 2016/11/03 16:43:15, Sami wrote: > > Could we instead cache the previous result and return that here? I think it > > might be a bit surprising to randomly switch our answer depending on if we got > > lucky with the critical section or not. > > I'd prefer we err on the side of not intervening, but I don't feel strongly > about it. > > In practice, I don't think we'll run into this often at all anyways (especially > when there are long tasks). It doesn't seem worth the extra (small amount of) > complexity. > > Do you think this might actually matter? I'm thinking consistency is worth the one extra boolean we need to keep around for this :) Especially if we start using this for other things -- what's appropriate for adjusting input event blocking behavior might not be the same for something else. https://codereview.chromium.org/2273703002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1576: seqlock_queueing_time_estimator_.seqlock.WriteBegin(); On 2016/11/03 18:23:54, tdresser wrote: > On 2016/11/03 16:43:15, Sami wrote: > > Is there a way to do this write less often, e.g., only when the answer > changes? > > What do you mean by "the answer"? The most recent task end time will always have > changed, and we require that knowledge for computing the EQT. Yeah, I just realized after I wrote that that this doesn't quite work that way. Basically we're computing an estimate that flips between true and false very rarely, so it seems a little odd that we need this high fidelity data streaming to make it tick. I don't have a better suggestion at the moment, so this is probably fine for now.
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added some tests, and fixed a bug or two. PTAL
https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:12: #include <array> Is this needed? https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1521: base::TimeTicks now = tick_clock()->NowTicks(); Is it worth adding a TRACE_EVENT here? https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:437: AnyThread(RendererSchedulerImpl* renderer_scheduler_impl); explicit
https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:62: QueueingTimeEstimator::QueueingTimeEstimator(const Data& data) : data_(data) {} client_ is not initialized here. Looking at the way this is used below, should we pass in QueueingTimeEstimator::Client as a parameter too? https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:71: if (data_.window_start_time.is_null()) bike-shed: all these data_.'s make me wonder if this method (and the one above) should be moved to Data. I suspect that'd let temporary_queueing_time_estimator be of type Data. Of course that suggests Data is possibly now a bad name. I don't have any great alternative names beyond 'State' https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:97: void OnQueueingTimeForWindowEstimated( // QueueingTimeEstimator::Client implementation:
Thanks Tim, I think the threading logic looks solid now. Couple of minor niggles. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:46: Data data() const { return data_; } nit: I would make this return a const Data& and have the client be responsible for copying the struct. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (left): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:252: // If a task was deferred, try again with another task. I think this comment is still valid? https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1549: { nit: any reason for these braces? We're not grabbing any locks here. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:387: SeqLockQueueingTimeEstimator seqlock_queueing_time_estimator_; Please add a short note here saying that this only protects access to the data() method of the estimator. I was trying to think of a way to have this be a ::Data instead, but I couldn't think of a straightforward way (unless we make the class that is signalling the client be able to use external storage for Data somehow). https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:504: base::TimeDelta main_thread_responsiveness_threshold_; Do you still need this given the constant is a TimeDelta now too? (and the test is using the same default value of 200ms)
The CQ bit was checked by tdresser@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the massive patch latency in this CL... Now that you've completely forgotten what we were doing, here's another revision. :) https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:62: QueueingTimeEstimator::QueueingTimeEstimator(const Data& data) : data_(data) {} On 2016/11/14 17:09:35, alex clarke wrote: > client_ is not initialized here. > > Looking at the way this is used below, should we pass in > QueueingTimeEstimator::Client as a parameter too? We use this contructor in RendererSchedulerImpl not below. In that case, we don't need a client. Fixed not initializing client_. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:71: if (data_.window_start_time.is_null()) On 2016/11/14 17:09:35, alex clarke wrote: > bike-shed: all these data_.'s make me wonder if this method (and the one above) > should be moved to Data. > > I suspect that'd let temporary_queueing_time_estimator be of type Data. > > Of course that suggests Data is possibly now a bad name. I don't have any great > alternative names beyond 'State' Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:97: void OnQueueingTimeForWindowEstimated( On 2016/11/14 17:09:35, alex clarke wrote: > // QueueingTimeEstimator::Client implementation: Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:12: #include <array> On 2016/11/14 17:00:17, alex clarke wrote: > Is this needed? Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:46: Data data() const { return data_; } On 2016/11/14 21:16:45, Sami wrote: > nit: I would make this return a const Data& and have the client be responsible > for copying the struct. Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (left): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:252: // If a task was deferred, try again with another task. On 2016/11/14 21:16:45, Sami wrote: > I think this comment is still valid? Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1521: base::TimeTicks now = tick_clock()->NowTicks(); On 2016/11/14 17:00:17, alex clarke wrote: > Is it worth adding a TRACE_EVENT here? For when we're checking if the main thread is unresponsive? What's the goal? (Not saying we shouldn't, I'm just not clear on why we should.) https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1549: { On 2016/11/14 21:16:45, Sami wrote: > nit: any reason for these braces? We're not grabbing any locks here. Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:387: SeqLockQueueingTimeEstimator seqlock_queueing_time_estimator_; On 2016/11/14 21:16:45, Sami wrote: > Please add a short note here saying that this only protects access to the data() > method of the estimator. I was trying to think of a way to have this be a ::Data > instead, but I couldn't think of a straightforward way (unless we make the class > that is signalling the client be able to use external storage for Data somehow). Why do you say it only protects access to the data() method of the estimator? The SharedMemorySeqlockBuffer's data attribute is protected, which is the whole QueueingTimeEstimatorObject. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:437: AnyThread(RendererSchedulerImpl* renderer_scheduler_impl); On 2016/11/14 17:00:17, alex clarke wrote: > explicit Done. https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:504: base::TimeDelta main_thread_responsiveness_threshold_; On 2016/11/14 21:16:45, Sami wrote: > Do you still need this given the constant is a TimeDelta now too? (and the test > is using the same default value of 200ms) There's nothing worse than trying to land a change to a constant, and realizing it makes all your tests fail. I'd prefer we keep the test value independent, so that when we decide to tweak this, we don't need to update a bunch of tests.
+altimin as fyi
I think this looks OK now, Sami WDYT? https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:226: RendererSchedulerImpl* renderer_scheduler_impl) nit: this seems unused - lets revert this change. https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:188: // Returns whether or not the main thread appears unresponsive, based on the Please mention it can be called from any thread.
Thanks Alex. https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:226: RendererSchedulerImpl* renderer_scheduler_impl) On 2016/12/15 14:24:37, alex clarke wrote: > nit: this seems unused - lets revert this change. Done. https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:188: // Returns whether or not the main thread appears unresponsive, based on the On 2016/12/15 14:24:37, alex clarke wrote: > Please mention it can be called from any thread. Done.
lgtm, thanks for your patience! https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:387: SeqLockQueueingTimeEstimator seqlock_queueing_time_estimator_; On 2016/12/15 13:17:00, tdresser wrote: > On 2016/11/14 21:16:45, Sami wrote: > > Please add a short note here saying that this only protects access to the > data() > > method of the estimator. I was trying to think of a way to have this be a > ::Data > > instead, but I couldn't think of a straightforward way (unless we make the > class > > that is signalling the client be able to use external storage for Data > somehow). > > Why do you say it only protects access to the data() method of the estimator? > The SharedMemorySeqlockBuffer's data attribute is protected, which is the whole > QueueingTimeEstimatorObject. I'm a little fuzzy on what I was going for here, but it may have been the fact that this isn't an actual lock that lets two different parties mutate the object concurrently. I now think it should be obvious enough since we have "seqlock" in the name. https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:295: DCHECK(delegate_->IsNested()); This change seems unrelated? https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:189: // length and frequency of recent main thread tasks. Can be called from any The implementation seems to limit this to the compositor thread. I think that's fine, but let's make sure this comment matches.
tdresser@chromium.org changed reviewers: + isherman@chromium.org, jbroman@chromium.org, jochen@chromium.org, rockot@chromium.org
+jochen@ for content/public content/renderer/render_widget_unittest.cc third_party/WebKit/public/platform/WebInputEvent.h +rockot@ for device +isherman@ for tools/metrics/histograms/histograms.xml +jbroman@ for third_party/WebKit/Source/platform/BUILD.gn https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2273703002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:387: SeqLockQueueingTimeEstimator seqlock_queueing_time_estimator_; On 2016/12/15 17:54:37, Sami wrote: > On 2016/12/15 13:17:00, tdresser wrote: > > On 2016/11/14 21:16:45, Sami wrote: > > > Please add a short note here saying that this only protects access to the > > data() > > > method of the estimator. I was trying to think of a way to have this be a > > ::Data > > > instead, but I couldn't think of a straightforward way (unless we make the > > class > > > that is signalling the client be able to use external storage for Data > > somehow). > > > > Why do you say it only protects access to the data() method of the estimator? > > The SharedMemorySeqlockBuffer's data attribute is protected, which is the > whole > > QueueingTimeEstimatorObject. > > I'm a little fuzzy on what I was going for here, but it may have been the fact > that this isn't an actual lock that lets two different parties mutate the object > concurrently. I now think it should be obvious enough since we have "seqlock" in > the name. Done. https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:295: DCHECK(delegate_->IsNested()); On 2016/12/15 17:54:37, Sami wrote: > This change seems unrelated? Done. https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2273703002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:189: // length and frequency of recent main thread tasks. Can be called from any On 2016/12/15 17:54:38, Sami wrote: > The implementation seems to limit this to the compositor thread. I think that's > fine, but let's make sure this comment matches. Done.
Source/platform/BUILD.gn lgtm
device lgtm https://codereview.chromium.org/2273703002/diff/280001/device/base/synchroniz... File device/base/synchronization/one_writer_seqlock.h (right): https://codereview.chromium.org/2273703002/diff/280001/device/base/synchroniz... device/base/synchronization/one_writer_seqlock.h:35: void ReadOrFail(bool* can_read, base::subtle::Atomic32* version) const; nitty nit: Maybe TryRead is a better name. There isn't really a "fail" action that takes place so the name might be misleading.
https://codereview.chromium.org/2273703002/diff/280001/device/base/synchroniz... File device/base/synchronization/one_writer_seqlock.h (right): https://codereview.chromium.org/2273703002/diff/280001/device/base/synchroniz... device/base/synchronization/one_writer_seqlock.h:35: void ReadOrFail(bool* can_read, base::subtle::Atomic32* version) const; On 2016/12/15 19:20:37, Ken Rockot wrote: > nitty nit: Maybe TryRead is a better name. There isn't really a "fail" action > that takes place so the name might be misleading. Done.
Metrics LGTM % comments: https://codereview.chromium.org/2273703002/diff/300001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/2273703002/diff/300001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:181: GetEventLatencyMicros(event_timestamp, now), 1, 10000000, 100); How valuable is it to have 100 buckets for these histograms? In general, we encourage histograms of 50 buckets or fewer, to reduce the costs of recording such metrics. https://codereview.chromium.org/2273703002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2273703002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:14962: + tracks the benefit of forcing events non-blocking during fling. nit: I think you meant something about unresponsive main thread, rather than "during fling", on this line?
lgtm
https://codereview.chromium.org/2273703002/diff/300001/content/renderer/input... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/2273703002/diff/300001/content/renderer/input... content/renderer/input/render_widget_input_handler.cc:181: GetEventLatencyMicros(event_timestamp, now), 1, 10000000, 100); On 2016/12/16 00:46:02, Ilya Sherman wrote: > How valuable is it to have 100 buckets for these histograms? In general, we > encourage histograms of 50 buckets or fewer, to reduce the costs of recording > such metrics. Reduced to 50 buckets. At some point I'll need to go through here and rename a bunch of things and give them fewer buckets... Thanks for catching this. https://codereview.chromium.org/2273703002/diff/300001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2273703002/diff/300001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:14962: + tracks the benefit of forcing events non-blocking during fling. On 2016/12/16 00:46:02, Ilya Sherman wrote: > nit: I think you meant something about unresponsive main thread, rather than > "during fling", on this line? Done.
The CQ bit was checked by tdresser@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, lanwei@chromium.org, skyostil@chromium.org, rockot@chromium.org, jbroman@chromium.org, jochen@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2273703002/#ps320001 (title: "Histogram nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1481895298174950,
"parent_rev": "4426a074c7ba511a660b68cb18dcbe281bf0e030", "commit_rev":
"825645a74013d3adc298c1e1bd25733ec24ea6da"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Review-Url: https://codereview.chromium.org/2273703002 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 Review-Url: https://codereview.chromium.org/2273703002 ========== to ========== 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 Committed: https://crrev.com/2c6c355f995d54c481419acebed7f988207c57c4 Cr-Commit-Position: refs/heads/master@{#439106} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/2c6c355f995d54c481419acebed7f988207c57c4 Cr-Commit-Position: refs/heads/master@{#439106} |
