Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2265873004:
[scheduler] Monitor renderer load level. (Closed)
|
Created:
4 years, 4 months ago by altimin Modified:
4 years, 4 months ago CC:
chromium-reviews, blink-reviews, scheduler-bugs_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[scheduler] Monitor renderer main thread load level.
Add monitoring of main thread load level for background and foreground renderers.
BUG=639852
Committed: https://crrev.com/3ab9c0bc90a06439f5c2173b8a2ba3448d3b819a
Cr-Commit-Position: refs/heads/master@{#413783}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fixes according to comments #
Total comments: 12
Patch Set 3 : Added some comments #
Total comments: 6
Patch Set 4 : Address alexclarke@'s comments #Patch Set 5 : Addressed some comments #
Total comments: 2
Patch Set 6 : Addressed some comments #
Total comments: 2
Patch Set 7 : Address haraken@'s comments #Patch Set 8 : Fix windows compilation #Messages
Total messages: 36 (15 generated)
Description was changed from ========== [scheduler] Monitor renderer load level. Add monitoring of load level for background and foreground renderers. BUG= ========== to ========== [scheduler] Monitor renderer load level. Add monitoring of load level for background and foreground renderers. BUG= ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
This should give us some interesting numbers. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.cc:53: bool is_active, nit: The call sites would be a bit easier to read if these were enums. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:16: // This class tracks renderer load level. nit: This comment basically just repeats the class name :) How about also explaining what load level means? https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:18: class BLINK_PLATFORM_EXPORT RendererLoadTracker { Since this only tracks the main thread instead of the whole renderer, should we call this something like ThreadLoadTracker? (And also move it next to the other estimators in ../base) https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:31: void RecordIdle(base::TimeTicks now); Could we also count wake-ups here? Could you add a TODO for that? https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:138: time_source->NowTicks(), It'd be nice to pass in the same starting time to both trackers :P https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1447: MainThreadOnly().foreground_renderer_load_tracker.RecordTaskTime(start_time, Could we only do this for the one active one? https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1450: end_time); Maybe add a comment saying we'd really want to use thread time instead of wall time but for efficiency reasons we're reusing the wall time measurement here?
PTAL https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.cc:53: bool is_active, On 2016/08/22 14:33:16, Sami wrote: > nit: The call sites would be a bit easier to read if these were enums. Done. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:16: // This class tracks renderer load level. On 2016/08/22 14:33:16, Sami wrote: > nit: This comment basically just repeats the class name :) How about also > explaining what load level means? Done. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:18: class BLINK_PLATFORM_EXPORT RendererLoadTracker { On 2016/08/22 14:33:16, Sami wrote: > Since this only tracks the main thread instead of the whole renderer, should we > call this something like ThreadLoadTracker? (And also move it next to the other > estimators in ../base) Done. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_load_tracker.h:31: void RecordIdle(base::TimeTicks now); On 2016/08/22 14:33:16, Sami wrote: > Could we also count wake-ups here? Could you add a TODO for that? Done. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:138: time_source->NowTicks(), On 2016/08/22 14:33:16, Sami wrote: > It'd be nice to pass in the same starting time to both trackers :P Done. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1447: MainThreadOnly().foreground_renderer_load_tracker.RecordTaskTime(start_time, On 2016/08/22 14:33:16, Sami wrote: > Could we only do this for the one active one? Started to bail out early from ThreadLoadTracker::Advance when thread is paused instead. https://codereview.chromium.org/2265873004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1450: end_time); On 2016/08/22 14:33:16, Sami wrote: > Maybe add a comment saying we'd really want to use thread time instead of wall > time but for efficiency reasons we're reusing the wall time measurement here? Done.
https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:8: const int kLoadReportingIntervalInMilliseconds = 1000; nit: any particular reason for using different units here? https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:14: const Callback& callback) nit: indent looks wrong in several places. https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:16: // This class tracks renderer load level, e.g. percentage of wall time spent nit: i.e. the percentage... https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:45: void Advance(base::TimeTicks now, TaskState task_state); Please add a comment explaining what this does. You should probably mention that the Callback could be issued multiple times. bike-shed: If there was a very long interval between calling Advance, the callback might be issued many times (assuming I'm reading the code right). Are we comfortable with that? If not should we change the Callback to specify how long it's been it's been in the current state? https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:49: base::TimeTicks current_time_; Nit: This can't actually be the current time since the moment it's recorded it's out of date ;) https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:460: MainThreadOnly().background_main_thread_load_tracker.Pause(now); I wonder if you also want to add something in WebViewSchedulerImpl?
lgtm once Alex is happy. https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:67: while (true) { nit: while (current_time < now) would make this look a bit less scary :) https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:16: // This class tracks renderer load level, e.g. percentage of wall time spent s/renderer/thread/ Also since this class doesn't actually do the time lookup, we don't know here whether we're dealing with wall time or not. https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: UMA_HISTOGRAM_PERCENTAGE("RendererScheduler.ForegroundRendererLoad", Please consider adding a disabled-by-default trace counter for this too -- makes it easier to debug things later.
(BTW please also link this to your bug)
PTAL https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:8: const int kLoadReportingIntervalInMilliseconds = 1000; On 2016/08/22 16:13:13, alex clarke wrote: > nit: any particular reason for using different units here? Initially it was 100, but I thought that it's a bad idea after some time. Thanks for spotting. https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:14: const Callback& callback) On 2016/08/22 16:13:13, alex clarke wrote: > nit: indent looks wrong in several places. I've run "git cl format", but it seems to work strangely. Please point out errors when you see them. https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:16: // This class tracks renderer load level, e.g. percentage of wall time spent On 2016/08/22 16:13:13, alex clarke wrote: > nit: i.e. the percentage... I meant that we also are going to track wakeups and other things :) https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:45: void Advance(base::TimeTicks now, TaskState task_state); On 2016/08/22 16:13:13, alex clarke wrote: > Please add a comment explaining what this does. You should probably mention > that the Callback could be issued multiple times. > > bike-shed: If there was a very long interval between calling Advance, the > callback might be issued many times (assuming I'm reading the code right). Are > we comfortable with that? If not should we change the Callback to specify how > long it's been it's been in the current state? Comment added (also commented source in .cc file). Yes, that was intentional to call callback multiple times here. We can't update metrics based on tasks being run due to bias from task frequency (we will have more data points when more tasks are run) and we can't post task to take metrics every second due to obvious reasons (overhead, we do not want to wake up renderer, etc). So here we're emulating callback being called every second, even with delays. base::TimeTicks parameter in callback helps to address this problem with delays. https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:49: base::TimeTicks current_time_; On 2016/08/22 16:13:13, alex clarke wrote: > Nit: This can't actually be the current time since the moment it's recorded it's > out of date ;) Done. https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:460: MainThreadOnly().background_main_thread_load_tracker.Pause(now); On 2016/08/22 16:13:13, alex clarke wrote: > I wonder if you also want to add something in WebViewSchedulerImpl? As discussed offline, no due to renderer hosting a very limited number of tabs. https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:67: while (true) { On 2016/08/22 16:35:40, Sami wrote: > nit: while (current_time < now) would make this look a bit less scary :) Done. https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:16: // This class tracks renderer load level, e.g. percentage of wall time spent On 2016/08/22 16:35:40, Sami wrote: > s/renderer/thread/ > > Also since this class doesn't actually do the time lookup, we don't know here > whether we're dealing with wall time or not. Replacement done. Actually, given that we process tasks as (start_time, end_time) interval, we're using wall time here. https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2265873004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:41: UMA_HISTOGRAM_PERCENTAGE("RendererScheduler.ForegroundRendererLoad", On 2016/08/22 16:35:40, Sami wrote: > Please consider adding a disabled-by-default trace counter for this too -- makes > it easier to debug things later. Done, thanks!
Description was changed from ========== [scheduler] Monitor renderer load level. Add monitoring of load level for background and foreground renderers. BUG= ========== to ========== [scheduler] Monitor renderer load level. Add monitoring of load level for background and foreground renderers. BUG=639852 ==========
lgtm
altimin@chromium.org changed reviewers: + haraken@chromium.org, mpearson@chromium.org
+mpearson@ for histograms.xml +haraken@ for blink_platform.gypi PTAL.
https://codereview.chromium.org/2265873004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2265873004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47581: + tasks. Every histogram should mention when it is emitted. Also, it would be good to clarity how this is affected by number of cores. Usually, say, a 4-core machine can go up to a load of 4, yet this description of yours implies it can only go up to a "load" of one because you measure load as percent of time. Which is it? (Depending on your answer, you may also want to revise the histogram name.)
PTAL https://codereview.chromium.org/2265873004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2265873004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47581: + tasks. On 2016/08/22 17:28:38, Mark P wrote: > Every histogram should mention when it is emitted. > > Also, it would be good to clarity how this is affected by number of cores. > Usually, say, a 4-core machine can go up to a load of 4, yet this description of > yours implies it can only go up to a "load" of one because you measure load as > percent of time. Which is it? (Depending on your answer, you may also want to > revise the histogram name.) Actually, it's main thread load. Changed name to reflect this.
histograms.xml lgtm
LGTM https://codereview.chromium.org/2265873004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2265873004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:1: #include "platform/scheduler/base/thread_load_tracker.h" Add a copyright. https://codereview.chromium.org/2265873004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2265873004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:41: // This functions advances |time_| to |now|, calling |callback_| function
Description was changed from ========== [scheduler] Monitor renderer load level. Add monitoring of load level for background and foreground renderers. BUG=639852 ========== to ========== [scheduler] Monitor renderer main thread load level. Add monitoring of main thread load level for background and foreground renderers. BUG=639852 ==========
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, alexclarke@chromium.org, haraken@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2265873004/#ps120001 (title: "Address haraken@'s comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, mpearson@chromium.org, skyostil@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2265873004/#ps140001 (title: "Fix windows compilation")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by altimin@chromium.org
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Monitor renderer main thread load level. Add monitoring of main thread load level for background and foreground renderers. BUG=639852 ========== to ========== [scheduler] Monitor renderer main thread load level. Add monitoring of main thread load level for background and foreground renderers. BUG=639852 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Monitor renderer main thread load level. Add monitoring of main thread load level for background and foreground renderers. BUG=639852 ========== to ========== [scheduler] Monitor renderer main thread load level. Add monitoring of main thread load level for background and foreground renderers. BUG=639852 Committed: https://crrev.com/3ab9c0bc90a06439f5c2173b8a2ba3448d3b819a Cr-Commit-Position: refs/heads/master@{#413783} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/3ab9c0bc90a06439f5c2173b8a2ba3448d3b819a Cr-Commit-Position: refs/heads/master@{#413783} |
