Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391593002:
[scheduler] Change ThreadLoadTracker to use only recent data. (Closed)
Description[scheduler] Change ThreadLoadTracker to use only recent data.
Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment.
This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load.
BUG=639852
Committed: https://crrev.com/4ec676871d9e7963b3575cd76fda00d8077b7fe1
Cr-Commit-Position: refs/heads/master@{#423509}
Patch Set 1 #Patch Set 2 : Simplified #
Total comments: 26
Patch Set 3 : Addressed comments from alexclarke@ and skyostil@ #Patch Set 4 : Fix #Patch Set 5 : Changed reporting interval to 1 minute #Patch Set 6 : Fixed android compilation #Patch Set 7 : Rebased #
Messages
Total messages: 44 (20 generated)
Description was changed from ========== [scheduler] Change ThreadLoadTracker to use only recent data. Switch ThreadLoadTracker from reporting average loads on the intervals from the beginning to current moment to reporting average loads from sliding window of one second width. BUG=639852 ========== to ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL
tdresser@chromium.org changed reviewers: + tdresser@chromium.org
We're going to want data on the tasks in the last n seconds for Expected Queueing Time computation as well. When Deep and I were discussing it, we thought that keeping a queue with a minute's worth of task lengths could grow overly long, so we discussed approaches for bounding it's length. Do we not need to worry about the queue length?
PTAL. Following Tim's question I realized that we do not need to store all tasks, buckets will do. Regarding storing tasks durations — AFAIK, we're already storing 1000 last tasks to get some task duration estimations. Maybe Sami or Alex will have an opinion on that.
https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:94: // Forward time_ to the earliest of following: nit pick: s/Forward/Advance (here and below) https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:101: // Forward time and recalculate |recorded_time_| and maybe: Keep a running total of the time spent running tasks within the window and the total time. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:103: if (thread_state_ == ThreadState::ACTIVE) { Currently thread_state_ is always ThreadState::ACTIVE if we got here. I suppose callback_.Run might change it, although that would be weird. How about removing this if and on line 116 and put DCHECK(thread_state_ == ThreadState::ACTIVE); after the callback_.Run()? This would make the code a bit simpler. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:8: #include <queue> Looks like this is no longer used. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:21: // It reports every |reporting_interval_| average thread load level in Every |reporting_interval_| time units, it reports the average thread load level computed using a sliding window of width |reporting_interval_|.
I think something like this makes sense to get a less biased view of the thread load. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:15: const int kWaitingPeriodBeforeReportingInSeconds = 10; The description talks about a one minute interval but this is still 10s? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:55: Meant to add this blank line? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:70: base::TimeTicks left = std::max(left1, left2); DCHECK_LT(left1, right1); DCHECK_LT(left2, right2); ? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:78: } } // namespace https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:83: if (time_ > now) { Random question: why would the time ever go backwards? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:108: next_reporting_time_, time_, time_ + delta); time_ + delta will never be later than next_reporting_time_ so it seems we only need the left side intersection here, right? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:56: base::TimeDelta recorded_time_; Would |active_time_| or maybe |total_active_time_| be a better name for this? https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:65: // Recorded run time in bucket s/bucket/window./? (Just wanted to avoid mixing these terms -- especially together with UMA.)
And re: timestamps, we do keep O(1000) of them in a couple of places already, so adding another one doesn't seem like a huge deal as long as we're smart about it.
Description was changed from ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ========== to ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last second average load. BUG=639852 ==========
PTAL https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:15: const int kWaitingPeriodBeforeReportingInSeconds = 10; On 2016/10/05 15:28:12, Sami wrote: > The description talks about a one minute interval but this is still 10s? Description is wrong, fixed. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:55: On 2016/10/05 15:28:12, Sami wrote: > Meant to add this blank line? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:70: base::TimeTicks left = std::max(left1, left2); On 2016/10/05 15:28:12, Sami wrote: > DCHECK_LT(left1, right1); > DCHECK_LT(left2, right2); ? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:78: } On 2016/10/05 15:28:12, Sami wrote: > } // namespace Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:83: if (time_ > now) { On 2016/10/05 15:28:12, Sami wrote: > Random question: why would the time ever go backwards? I was afraid of renderer going offscreen / onscreen in the middle of a task, but now realized that it should not be possible. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:94: // Forward time_ to the earliest of following: On 2016/10/05 15:25:22, alex clarke wrote: > nit pick: s/Forward/Advance (here and below) Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:101: // Forward time and recalculate |recorded_time_| and On 2016/10/05 15:25:22, alex clarke wrote: > maybe: Keep a running total of the time spent running tasks within the window > and the total time. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:103: if (thread_state_ == ThreadState::ACTIVE) { On 2016/10/05 15:25:23, alex clarke wrote: > Currently thread_state_ is always ThreadState::ACTIVE if we got here. > > I suppose callback_.Run might change it, although that would be weird. How > about removing this if and on line 116 and put DCHECK(thread_state_ == > ThreadState::ACTIVE); after the callback_.Run()? > > This would make the code a bit simpler. Very well noticed, thanks! https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:108: next_reporting_time_, time_, time_ + delta); On 2016/10/05 15:28:12, Sami wrote: > time_ + delta will never be later than next_reporting_time_ so it seems we only > need the left side intersection here, right? Yes, but I believe that a simple intersection is easier to read. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:8: #include <queue> On 2016/10/05 15:25:23, alex clarke wrote: > Looks like this is no longer used. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:21: // It reports every |reporting_interval_| average thread load level in On 2016/10/05 15:25:23, alex clarke wrote: > Every |reporting_interval_| time units, it reports the average thread load level > computed using a sliding window of width |reporting_interval_|. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:56: base::TimeDelta recorded_time_; On 2016/10/05 15:28:13, Sami wrote: > Would |active_time_| or maybe |total_active_time_| be a better name for this? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:65: // Recorded run time in bucket On 2016/10/05 15:28:13, Sami wrote: > s/bucket/window./? (Just wanted to avoid mixing these terms -- especially > together with UMA.) Done.
PTAL https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:15: const int kWaitingPeriodBeforeReportingInSeconds = 10; On 2016/10/05 15:28:12, Sami wrote: > The description talks about a one minute interval but this is still 10s? Description is wrong, fixed. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:55: On 2016/10/05 15:28:12, Sami wrote: > Meant to add this blank line? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:70: base::TimeTicks left = std::max(left1, left2); On 2016/10/05 15:28:12, Sami wrote: > DCHECK_LT(left1, right1); > DCHECK_LT(left2, right2); ? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:78: } On 2016/10/05 15:28:12, Sami wrote: > } // namespace Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:83: if (time_ > now) { On 2016/10/05 15:28:12, Sami wrote: > Random question: why would the time ever go backwards? I was afraid of renderer going offscreen / onscreen in the middle of a task, but now realized that it should not be possible. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:94: // Forward time_ to the earliest of following: On 2016/10/05 15:25:22, alex clarke wrote: > nit pick: s/Forward/Advance (here and below) Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:101: // Forward time and recalculate |recorded_time_| and On 2016/10/05 15:25:22, alex clarke wrote: > maybe: Keep a running total of the time spent running tasks within the window > and the total time. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:103: if (thread_state_ == ThreadState::ACTIVE) { On 2016/10/05 15:25:23, alex clarke wrote: > Currently thread_state_ is always ThreadState::ACTIVE if we got here. > > I suppose callback_.Run might change it, although that would be weird. How > about removing this if and on line 116 and put DCHECK(thread_state_ == > ThreadState::ACTIVE); after the callback_.Run()? > > This would make the code a bit simpler. Very well noticed, thanks! https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.cc:108: next_reporting_time_, time_, time_ + delta); On 2016/10/05 15:28:12, Sami wrote: > time_ + delta will never be later than next_reporting_time_ so it seems we only > need the left side intersection here, right? Yes, but I believe that a simple intersection is easier to read. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h (right): https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:8: #include <queue> On 2016/10/05 15:25:23, alex clarke wrote: > Looks like this is no longer used. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:21: // It reports every |reporting_interval_| average thread load level in On 2016/10/05 15:25:23, alex clarke wrote: > Every |reporting_interval_| time units, it reports the average thread load level > computed using a sliding window of width |reporting_interval_|. Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:56: base::TimeDelta recorded_time_; On 2016/10/05 15:28:13, Sami wrote: > Would |active_time_| or maybe |total_active_time_| be a better name for this? Done. https://codereview.chromium.org/2391593002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/thread_load_tracker.h:65: // Recorded run time in bucket On 2016/10/05 15:28:13, Sami wrote: > s/bucket/window./? (Just wanted to avoid mixing these terms -- especially > together with UMA.) Done.
A one second window feels pretty short and noisy. For example Linux uses 1m, 5m and 15m. Should we use something longer too or is there a good reason to go with 1s?
Changed interval to 1 minute. PTAL
lgtm. Please update the patch description too.
Description was changed from ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last second average load. BUG=639852 ========== to ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ==========
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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 Link to the patchset: https://codereview.chromium.org/2391593002/#ps100001 (title: "Fixed android 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_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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 Link to the patchset: https://codereview.chromium.org/2391593002/#ps120001 (title: "Rebased")
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] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ========== to ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 ========== to ========== [scheduler] Change ThreadLoadTracker to use only recent data. Currently ThreadLoadTracker accumulates data from the start and reports average load of an interval from the start to current moment. This patch replaces cumulative approach with sliding window and makes ThreadLoadTracker report last minute average load. BUG=639852 Committed: https://crrev.com/4ec676871d9e7963b3575cd76fda00d8077b7fe1 Cr-Commit-Position: refs/heads/master@{#423509} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4ec676871d9e7963b3575cd76fda00d8077b7fe1 Cr-Commit-Position: refs/heads/master@{#423509} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
