Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2866613002:
EQT: Change Expected Queuing Time from per-second to sliding window (Closed)
|
Created:
3 years, 7 months ago by Liquan (Max) Gu Modified:
3 years, 7 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch, scheduler-bugs_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEQT: Change Expected Queuing Time from per-second to sliding window
In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we
need to extend the current EQT from the per-second approach to a sliding window
approach. The change makes the original per-second use case a special case of
the sliding window use cases.
BUG=710449
Review-Url: https://codereview.chromium.org/2866613002
Cr-Commit-Position: refs/heads/master@{#471803}
Committed: https://chromium.googlesource.com/chromium/src/+/7a8f8e51e1e0d43cb0a24a629d508bc473864a28
Patch Set 1 #
Total comments: 29
Patch Set 2 : Initilize buffer size at initialization; add documentation to ratio and add illustration of sliding… #
Total comments: 29
Patch Set 3 : Fix EQTCurrentTask; sliding window and test coverage for renderer_scheduler #
Total comments: 20
Patch Set 4 : uma report per window duration; sliding window include the starting points #
Total comments: 18
Patch Set 5 : del clearBuffer #
Total comments: 1
Patch Set 6 : nit #
Total comments: 12
Patch Set 7 : function const; move uma_variable in MainThreadOnly #Patch Set 8 : rebase only #Patch Set 9 : escape backslash #Messages
Total messages: 41 (17 generated)
Description was changed from ========== sliding window expected queueing time Sliding window EQT BUG= ========== to ========== EQT: From per-second to sliding window Expected Queuing Time In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ==========
maxlg@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== EQT: From per-second to sliding window Expected Queuing Time In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ========== to ========== EQT: Change Expected Queuing Time from per-second to sliding window In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ==========
On 2017/05/05 00:19:40, Liquan (Max) Gu wrote: > Description was changed from > > ========== > EQT: From per-second to sliding window Expected Queuing Time > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we > need to extend the current EQT from the per-second approach to a sliding window > approach. The change makes the original per-second use case a special case of > the sliding window use cases. > > BUG=710449 > ========== > > to > > ========== > EQT: Change Expected Queuing Time from per-second to sliding window > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we > need to extend the current EQT from the per-second approach to a sliding window > approach. The change makes the original per-second use case a special case of > the sliding window use cases. > > BUG=710449 > ========== Just noticed this looking at my dashboard - this hasn't been published yet, let me know if I should take a look.
Yeah, I uploaded this just yesterday evening. It's ready to review now. On May 5, 2017 09:32, <tdresser@chromium.org> wrote: > On 2017/05/05 00:19:40, Liquan (Max) Gu wrote: > > Description was changed from > > > > ========== > > EQT: From per-second to sliding window Expected Queuing Time > > > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in > UMA, we > > need to extend the current EQT from the per-second approach to a sliding > window > > approach. The change makes the original per-second use case a special > case of > > the sliding window use cases. > > > > BUG=710449 > > ========== > > > > to > > > > ========== > > EQT: Change Expected Queuing Time from per-second to sliding window > > > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in > UMA, we > > need to extend the current EQT from the per-second approach to a sliding > window > > approach. The change makes the original per-second use case a special > case of > > the sliding window use cases. > > > > BUG=710449 > > ========== > > Just noticed this looking at my dashboard - this hasn't been published > yet, let > me know if I should take a look. > > https://codereview.chromium.org/2866613002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Yeah, I uploaded this just yesterday evening. It's ready to review now. On May 5, 2017 09:32, <tdresser@chromium.org> wrote: > On 2017/05/05 00:19:40, Liquan (Max) Gu wrote: > > Description was changed from > > > > ========== > > EQT: From per-second to sliding window Expected Queuing Time > > > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in > UMA, we > > need to extend the current EQT from the per-second approach to a sliding > window > > approach. The change makes the original per-second use case a special > case of > > the sliding window use cases. > > > > BUG=710449 > > ========== > > > > to > > > > ========== > > EQT: Change Expected Queuing Time from per-second to sliding window > > > > In order to record Sliding Window Expected Queuing Time(EQT) Metric in > UMA, we > > need to extend the current EQT from the per-second approach to a sliding > window > > approach. The change makes the original per-second use case a special > case of > > the sliding window use cases. > > > > BUG=710449 > > ========== > > Just noticed this looking at my dashboard - this hasn't been published > yet, let > me know if I should take a look. > > https://codereview.chromium.org/2866613002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks - mainly style and naming nits. I'll want another pass on this once these have been addressed. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:97: DLOG(WARNING) << "OnTopLevelTaskCompleted()\n"; Remove. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:162: running_sum -= circular_buffer[next_index]; Looks like |next_index| is being used as the current index. Should it just be named |index_| or |current_index_|? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:211: .slide_step_expected_queueing_time); Won't this be the value for a single step, instead of the whole window? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:32: class CircularBuffer { The important thing about this class isn't that it's a circular buffer, but that it computes a running average. Maybe something like: class RunningAverage with methods Add and GetAverage? ? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:35: ~CircularBuffer() { circular_buffer.clear(); } The circular_buffer will be deallocated when this object is destroyed, so we don't need to clear it. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:36: void SetSize(int size); Should we ever be able to change the size of the buffer? Could it be a parameter to the constructor? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:38: void ReplaceNext(base::TimeDelta binValue); bin_value https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:42: int next_index; Nit: " Data members of classes (but not structs) additionally have trailing underscores. " https://google.github.io/styleguide/cppguide.html#Variable_Names Should be: next_index_, circular_buffer_ etc. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:53: base::TimeDelta current_expected_queueing_time; Are we still using this? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:54: base::TimeDelta slide_step_expected_queueing_time; The existing code perhaps isn't a great example, but lets add a few comments on these fields. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:55: base::TimeDelta window_duration; Are we still using this? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:60: CircularBuffer step_queueing_time_buffer; Oops, I've broken the style rules here, which probably resulted in some of the breaking of style rules that you did. Technically these should all be private, and have getters and setters. It's fine by me if you're consistent here though, and don't bother fixing this. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:69: int window_duration_step_ratio); Let's add some documentation on what window_duration_step_ratio is.
https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:97: DLOG(WARNING) << "OnTopLevelTaskCompleted()\n"; On 2017/05/05 14:19:15, tdresser wrote: > Remove. Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:162: running_sum -= circular_buffer[next_index]; On 2017/05/05 14:19:15, tdresser wrote: > Looks like |next_index| is being used as the current index. Should it just be > named |index_| or |current_index_|? By next I mean this is the next to the last filled bin of CircularBuffer. But yes, whether it's next or current depends on which line it runs to. I will just use index_. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:211: .slide_step_expected_queueing_time); On 2017/05/05 14:19:15, tdresser wrote: > Won't this be the value for a single step, instead of the whole window? Sorry I missed this part. The intention of this part is not very clear to me. Why it skips the last many windows and only return the EQT in the (N-1)th or Nth window? https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:32: class CircularBuffer { On 2017/05/05 14:19:16, tdresser wrote: > The important thing about this class isn't that it's a circular buffer, but that > it computes a running average. > > Maybe something like: > class RunningAverage > with methods Add and GetAverage? > > ? > Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:35: ~CircularBuffer() { circular_buffer.clear(); } On 2017/05/05 14:19:16, tdresser wrote: > The circular_buffer will be deallocated when this object is destroyed, so we > don't need to clear it. Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:36: void SetSize(int size); On 2017/05/05 14:19:16, tdresser wrote: > Should we ever be able to change the size of the buffer? Could it be a parameter > to the constructor? The size of it shouldn't need to be changed. I agree with you. I will turn it to be a pointer to implement it then. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:38: void ReplaceNext(base::TimeDelta binValue); On 2017/05/05 14:19:15, tdresser wrote: > bin_value Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:42: int next_index; On 2017/05/05 14:19:15, tdresser wrote: > Nit: " Data members of classes (but not structs) additionally have trailing > underscores. " > > https://google.github.io/styleguide/cppguide.html#Variable_Names > > Should be: next_index_, circular_buffer_ etc. Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:53: base::TimeDelta current_expected_queueing_time; On 2017/05/05 14:19:16, tdresser wrote: > Are we still using this? No, we won't use it any more. I will remore it. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:54: base::TimeDelta slide_step_expected_queueing_time; On 2017/05/05 14:19:16, tdresser wrote: > The existing code perhaps isn't a great example, but lets add a few comments on > these fields. Done. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:55: base::TimeDelta window_duration; On 2017/05/05 14:19:16, tdresser wrote: > Are we still using this? Yes we are still using window_duration, but I am thinking about removing window_slide_step_width, since it's always just act as a middle value equaling window_duration / window_duration_step_ratio; https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:60: CircularBuffer step_queueing_time_buffer; On 2017/05/05 14:19:15, tdresser wrote: > Oops, I've broken the style rules here, which probably resulted in some of the > breaking of style rules that you did. > > Technically these should all be private, and have getters and setters. It's fine > by me if you're consistent here though, and don't bother fixing this. Acknowledged. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:69: int window_duration_step_ratio); On 2017/05/05 14:19:16, tdresser wrote: > Let's add some documentation on what window_duration_step_ratio is. Is the documentation above ok? I am thinking whether other names will be better, such as num_steps_in_window?
https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:211: .slide_step_expected_queueing_time); On 2017/05/05 22:40:46, Liquan (Max) Gu wrote: > On 2017/05/05 14:19:15, tdresser wrote: > > Won't this be the value for a single step, instead of the whole window? > > Sorry I missed this part. The intention of this part is not very clear to me. > Why it skips the last many windows and only return the EQT in the (N-1)th or Nth > window? Yeah, this is ugly. The work you're doing will clean this up eventually. When we're including the current task, we're in this situation. | full window | unfinished window Ideally we'd report the EQT of the window ending where the unfinished window ends, but right now, we can only compute the EQT for windows aligned to the window size. Our solution is to take the maximum of the previous window, or the unfinished window. It's not great, but it's roughly similar. https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:55: base::TimeDelta window_duration; On 2017/05/05 22:40:47, Liquan (Max) Gu wrote: > On 2017/05/05 14:19:16, tdresser wrote: > > Are we still using this? > > Yes we are still using window_duration, but I am thinking about removing > window_slide_step_width, since it's always just act as a middle value equaling > window_duration / window_duration_step_ratio; That sounds reasonable. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:62: // window's step width, and the number of bins of the circular buffer. Wrap to 80 cols. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:70: state_.window_step_width = window_duration / window_duration_step_ratio; Getting rid of this (as you proposed) sounds good to me. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:107: for (int i = 0; i < step_queueing_times.GetSize() - 1; i++) { Nittiest nit: For partially historical reasons around the cost of incrementing iterators (http://stackoverflow.com/questions/1303899/performance-difference-between-ite...), we always preincrement iterators. i++ -> ++i https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:114: } I'm having a hard time following what's happening here. It looks like this can only process one task per step? and current_task_start_time isn't getting update here, so doesn't this add a the same task a bunch of times? https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:18: // randomly during each interval of length |window_duration| readd trailing . https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:47: State(int size); Let's name this steps_per_window as well. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:53: // By combining these step EQTs though running average, we can get window EQTs of a bigger window. This looks longer than our 80 column limit. I would have thought our presubmit hooks would catch this. Please wrap to 80 columns. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:69: // |------windowEQT_3------| Great diagram! https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:72: // |window_duration_step_ratio| = 3 because each window is in 3 steps' size. Wording nit: "3 because each window is in 3 steps' size." -> "3, because each window is the length of 3 steps." or something like that. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:74: base::TimeDelta slide_step_expected_queueing_time; I'd just call this "step_expected_queueing_time" or maybe "current_step_expected_queueing_time". https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:79: int window_duration_step_ratio; Let's call this "steps_per_window". It makes it clearer which way the ratio is. The way it is currently, it isn't clear whether this is steps per window, or windows per step. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:109: QueueingTimeEstimator(this, base::TimeDelta::FromSeconds(1), 1)), We'll want this to behave the same with a step size other than 1, won't we? Can we change this to something other than 1 to get test coverage of this? Alternatively, we should have a bunch more testing of the multi step path. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1673: QueueingTimeEstimator::State queueing_time_estimator_state(1); Same as above.
https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:211: .slide_step_expected_queueing_time); On 2017/05/08 17:58:14, tdresser wrote: > On 2017/05/05 22:40:46, Liquan (Max) Gu wrote: > > On 2017/05/05 14:19:15, tdresser wrote: > > > Won't this be the value for a single step, instead of the whole window? > > > > Sorry I missed this part. The intention of this part is not very clear to me. > > Why it skips the last many windows and only return the EQT in the (N-1)th or > Nth > > window? > > Yeah, this is ugly. The work you're doing will clean this up eventually. > > When we're including the current task, we're in this situation. > > | full window | unfinished window > Ideally we'd report the EQT of the window ending where the unfinished window > ends, but right now, we can only compute the EQT for windows aligned to the > window size. > > Our solution is to take the maximum of the previous window, or the unfinished > window. It's not great, but it's roughly similar. > > As discussed offline, we will compare the EQT of the last full window with that of the current partial window, and use the max of which to be the result. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:62: // window's step width, and the number of bins of the circular buffer. On 2017/05/08 17:58:14, tdresser wrote: > Wrap to 80 cols. Done. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:70: state_.window_step_width = window_duration / window_duration_step_ratio; On 2017/05/08 17:58:14, tdresser wrote: > Getting rid of this (as you proposed) sounds good to me. It's weird that it always fails once I substitute "window_step_width" with "(window_duration / steps_per_window)" or "(base::TimeDelta)(window_duration / steps_per_window)" or a temporary variable of base::TimeDelta type, so I just keep using window_step_width here. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:107: for (int i = 0; i < step_queueing_times.GetSize() - 1; i++) { On 2017/05/08 17:58:14, tdresser wrote: > Nittiest nit: > For partially historical reasons around the cost of incrementing iterators > (http://stackoverflow.com/questions/1303899/performance-difference-between-ite...), > we always preincrement iterators. > > i++ -> ++i Done. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:114: } On 2017/05/08 17:58:14, tdresser wrote: > I'm having a hard time following what's happening here. > > It looks like this can only process one task per step? and > current_task_start_time isn't getting update here, so doesn't this add a the > same task a bunch of times? Sorry this is a bug. The intent was to fill up all the bins at the beginning stage. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:18: // randomly during each interval of length |window_duration| On 2017/05/08 17:58:14, tdresser wrote: > readd trailing . Done. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:47: State(int size); On 2017/05/08 17:58:14, tdresser wrote: > Let's name this steps_per_window as well. This is more clear, thanks! https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:53: // By combining these step EQTs though running average, we can get window EQTs of a bigger window. On 2017/05/08 17:58:14, tdresser wrote: > This looks longer than our 80 column limit. I would have thought our presubmit > hooks would catch this. Please wrap to 80 columns. I've just installed the Clang-formatter that could help me with that, but it's not triggered on-save but manually. Not sure why presumit hooks doesn't work here though. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:69: // |------windowEQT_3------| On 2017/05/08 17:58:14, tdresser wrote: > Great diagram! Acknowledged. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:72: // |window_duration_step_ratio| = 3 because each window is in 3 steps' size. On 2017/05/08 17:58:14, tdresser wrote: > Wording nit: > > "3 because each window is in 3 steps' size." -> > "3, because each window is the length of 3 steps." > > or something like that. Done. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:74: base::TimeDelta slide_step_expected_queueing_time; On 2017/05/08 17:58:14, tdresser wrote: > I'd just call this "step_expected_queueing_time" or maybe > "current_step_expected_queueing_time". I think "step_expected_queueing_time" would be enough, since the class State has indicated it's a temporary state so I guess readers wouldn't confuse. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:79: int window_duration_step_ratio; On 2017/05/08 17:58:14, tdresser wrote: > Let's call this "steps_per_window". It makes it clearer which way the ratio is. > The way it is currently, it isn't clear whether this is steps per window, or > windows per step. Done.
https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:109: QueueingTimeEstimator(this, base::TimeDelta::FromSeconds(1), 1)), On 2017/05/08 17:58:15, tdresser wrote: > We'll want this to behave the same with a step size other than 1, won't we? Can > we change this to something other than 1 to get test coverage of this? > > Alternatively, we should have a bunch more testing of the multi step path. Does 5 steps per window sound good? https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1673: QueueingTimeEstimator::State queueing_time_estimator_state(1); On 2017/05/08 17:58:14, tdresser wrote: > Same as above. It seems unnecessary to define it seperately.
https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:70: state_.window_step_width = window_duration / window_duration_step_ratio; On 2017/05/10 15:08:26, Liquan (Max) Gu wrote: > On 2017/05/08 17:58:14, tdresser wrote: > > Getting rid of this (as you proposed) sounds good to me. > > It's weird that it always fails once I substitute "window_step_width" with > "(window_duration / steps_per_window)" or "(base::TimeDelta)(window_duration / > steps_per_window)" or a temporary variable of base::TimeDelta type, so I just > keep using window_step_width here. That's suspicious. I'd consider it worth taking a couple minutes to try to figure out why. https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:109: QueueingTimeEstimator(this, base::TimeDelta::FromSeconds(1), 1)), On 2017/05/10 15:14:21, Liquan (Max) Gu wrote: > On 2017/05/08 17:58:15, tdresser wrote: > > We'll want this to behave the same with a step size other than 1, won't we? > Can > > we change this to something other than 1 to get test coverage of this? > > > > Alternatively, we should have a bunch more testing of the multi step path. > > Does 5 steps per window sound good? > Hmmm. Eventually, we'll want something much greater than 5. Let's go with 20 for now? https://codereview.chromium.org/2866613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1673: QueueingTimeEstimator::State queueing_time_estimator_state(1); On 2017/05/10 15:14:21, Liquan (Max) Gu wrote: > On 2017/05/08 17:58:14, tdresser wrote: > > Same as above. > > It seems unnecessary to define it seperately. Acknowledged. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:63: // step width, and the number of bins of the circular buffer. Put documentation in the header. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:109: step_queueing_times.ClearBuffer(); Is this needed? Won't this have happened in the constructor? https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:129: step_queueing_times.GetAverage()); This will change our current behavior, right? The UMA will be reported way more frequently. Can we wrap the UMA here (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...) so it only reports once per second? You'll need to add the window start time to OnQueueingTimeForWindowEstimated. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:160: return filled_count_ == circular_buffer_.size(); We're adding some complexity here for a case which I don't think really matters. The question is, what's the expected queueing time in a window before this thread existed? You're currently assuming it's undefined, but I think we can call it 0, which simplifies the code a bit. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:220: // current partial window. Update comment. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:39: // Becasue taking average of the buffer requires all the bins to be filled, Becasue -> Because https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:84: // window's step width. The window must be a integer times the step's width. integer -> integer multiple of https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:93: int step_count; Is this used? https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3807: // 0.6324. Test looks good, I'm a bit lost in the comment though. In particular, the "for a task shorter than 1s, the length of a window," bit I'm finding confusing.
https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:63: // step width, and the number of bins of the circular buffer. On 2017/05/10 18:54:31, tdresser wrote: > Put documentation in the header. Done. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:109: step_queueing_times.ClearBuffer(); On 2017/05/10 18:54:31, tdresser wrote: > Is this needed? Won't this have happened in the constructor? This is put here in case we need to start another series of recording without destroying the object. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:129: step_queueing_times.GetAverage()); On 2017/05/10 18:54:31, tdresser wrote: > This will change our current behavior, right? The UMA will be reported way more > frequently. > > Can we wrap the UMA here > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/sched...) > so it only reports once per second? > > You'll need to add the window start time to OnQueueingTimeForWindowEstimated. Thanks for reminding me of this. There seems a lack of test coverage of this change as well. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:160: return filled_count_ == circular_buffer_.size(); On 2017/05/10 18:54:31, tdresser wrote: > We're adding some complexity here for a case which I don't think really matters. > > The question is, what's the expected queueing time in a window before this > thread existed? You're currently assuming it's undefined, but I think we can > call it 0, which simplifies the code a bit. Yes that's my intention, which is out of the consideration of keeping the behavior unchanged. I will change it with the test cases as well. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:220: // current partial window. On 2017/05/10 18:54:31, tdresser wrote: > Update comment. Done. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:39: // Becasue taking average of the buffer requires all the bins to be filled, On 2017/05/10 18:54:31, tdresser wrote: > Becasue -> Because Good finding! I was just curious how many people make the same error and then I found a lot in the code base. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:84: // window's step width. The window must be a integer times the step's width. On 2017/05/10 18:54:31, tdresser wrote: > integer -> integer multiple of Done. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:93: int step_count; On 2017/05/10 18:54:31, tdresser wrote: > Is this used? No, forgot to clear this up after using filled_time. https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3807: // 0.6324. On 2017/05/10 18:54:32, tdresser wrote: > Test looks good, I'm a bit lost in the comment though. > > In particular, the "for a task shorter than 1s, the length of a window," bit I'm > finding confusing. Sorry this should have used better wording.
This is almost there. Thanks for the great tests and diagrams! https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:109: step_queueing_times.ClearBuffer(); On 2017/05/11 00:57:28, Liquan (Max) Gu wrote: > On 2017/05/10 18:54:31, tdresser wrote: > > Is this needed? Won't this have happened in the constructor? > > This is put here in case we need to start another series of recording without > destroying the object. I don't think there's any way for us to reset window_start_time though, is there? https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:39: // Becasue taking average of the buffer requires all the bins to be filled, On 2017/05/11 00:57:29, Liquan (Max) Gu wrote: > On 2017/05/10 18:54:31, tdresser wrote: > > Becasue -> Because > > Good finding! I was just curious how many people make the same error and then I > found a lot in the code base. Some folks use comment spell checkers. I've never used one, as I find that so much of what we write in comments isn't real words that it's just annoying. It's not a huge problem if we get it wrong, but if we catch it, we might as well fix it. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:153: for (uint i = 0; i < circular_buffer_.size(); i++) { uint -> size_t https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... If you want to be fancy, you could do: for (size_t& bin : circular_buffer) bin = base::TimeDelta(); I don't think we actually need this method though. Everything but index_ is an object that will be default constructed, and we'll never need to clear this once we've gotten started. I think we can get away with just initializing index_ in the constructor. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:55: // smaller window of a step's width. By combining these step EQTs though though -> through a https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:183: EXPECT_EQ(base::TimeDelta::FromMilliseconds(3000), estimated_queueing_time); Can you add a comment explaining where this 3000 number came from? https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:216: EXPECT_EQ(base::TimeDelta::FromMilliseconds(25), estimated_queueing_time); Can you add a comment explaining where this 25 came from? https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:319: estimator.OnTopLevelTaskCompleted(time); I think a newline after the task completion calls makes these a bit easier to read. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:397: SlidingWindowOverTwoTasksStridingFirstWindow) { I'm not sure what "Striding First Window". https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1843: if (window_start_time - this->uma_last_report_window_start_time_ < this->uma_last_report_window_start_time_ -> uma_last_report_window_start_time_ https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:555: base::TimeTicks uma_last_report_window_start_time_; Let's make this be a bit more explicit. Maybe uma_last_queueing_time_report_window_start_time_?
https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:153: for (uint i = 0; i < circular_buffer_.size(); i++) { On 2017/05/11 13:52:44, tdresser wrote: > uint -> size_t > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > If you want to be fancy, you could do: > > for (size_t& bin : circular_buffer) > bin = base::TimeDelta(); > > I don't think we actually need this method though. Everything but index_ is an > object that will be default constructed, and we'll never need to clear this once > we've gotten started. > > I think we can get away with just initializing index_ in the constructor. Fixed Nice. Didn't realize C++ has this feature too, but shouldn't 'size_t' be 'base::TimeDelta'? for (base::TimeDelta& bin : circular_buffer) bin = base::TimeDelta(); https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:55: // smaller window of a step's width. By combining these step EQTs though On 2017/05/11 13:52:44, tdresser wrote: > though -> through a Done. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:183: EXPECT_EQ(base::TimeDelta::FromMilliseconds(3000), estimated_queueing_time); On 2017/05/11 13:52:45, tdresser wrote: > Can you add a comment explaining where this 3000 number came from? Done. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:216: EXPECT_EQ(base::TimeDelta::FromMilliseconds(25), estimated_queueing_time); On 2017/05/11 13:52:44, tdresser wrote: > Can you add a comment explaining where this 25 came from? Done. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:319: estimator.OnTopLevelTaskCompleted(time); On 2017/05/11 13:52:44, tdresser wrote: > I think a newline after the task completion calls makes these a bit easier to > read. Done. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:397: SlidingWindowOverTwoTasksStridingFirstWindow) { On 2017/05/11 13:52:45, tdresser wrote: > I'm not sure what "Striding First Window". Spanning several windows sound better? https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1843: if (window_start_time - this->uma_last_report_window_start_time_ < On 2017/05/11 13:52:45, tdresser wrote: > this->uma_last_report_window_start_time_ -> uma_last_report_window_start_time_ Right. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:555: base::TimeTicks uma_last_report_window_start_time_; On 2017/05/11 13:52:45, tdresser wrote: > Let's make this be a bit more explicit. > Maybe uma_last_queueing_time_report_window_start_time_? > Thanks! it's clearer.
tdresser@chromium.org changed reviewers: + skyostil@chromium.org
LGTM with nit. +skyostil for OWNERs review. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:153: for (uint i = 0; i < circular_buffer_.size(); i++) { On 2017/05/11 15:14:13, Liquan (Max) Gu wrote: > On 2017/05/11 13:52:44, tdresser wrote: > > uint -> size_t > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > > > If you want to be fancy, you could do: > > > > for (size_t& bin : circular_buffer) > > bin = base::TimeDelta(); > > > > I don't think we actually need this method though. Everything but index_ is an > > object that will be default constructed, and we'll never need to clear this > once > > we've gotten started. > > > > I think we can get away with just initializing index_ in the constructor. > > Fixed > > Nice. Didn't realize C++ has this feature too, but shouldn't 'size_t' be > 'base::TimeDelta'? > > for (base::TimeDelta& bin : circular_buffer) > bin = base::TimeDelta(); Whoops, you're right. https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:397: SlidingWindowOverTwoTasksStridingFirstWindow) { On 2017/05/11 15:14:13, Liquan (Max) Gu wrote: > On 2017/05/11 13:52:45, tdresser wrote: > > I'm not sure what "Striding First Window". > > Spanning several windows sound better? Yup! https://codereview.chromium.org/2866613002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc (right): https://codereview.chromium.org/2866613002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.cc:105: } Braces are optional if the condition and the body are both one line. Might as well not modify this.
Thanks for the improvement! lgtm with nits. Please also wrap the commit message at 72 columns. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:35: RunningAverage(int steps_per_window); Add explicit https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:36: int GetStepsPerWindow(); Let's make this const https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:38: base::TimeDelta GetAverage(); Ditto. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:35: uint window_duration_step_ratio) uint -> size_t (or just int) https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:308: // | |\ I love the diagrams! One thing to watch our for is that ending a // comment with a backslash means the following line is commented too and I think Clang may warn about that (http://stackoverflow.com/questions/30286253/how-to-escape-backslash-in-comment). If the bots are fine with this then no need to worry but I'd just thought I'd mention it. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:555: base::TimeTicks uma_last_queueing_time_report_window_start_time_; Please move this to the MainThreadOnly struct to make the threading constrains a little more obvious.
Description was changed from ========== EQT: Change Expected Queuing Time from per-second to sliding window In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ========== to ========== EQT: Change Expected Queuing Time from per-second to sliding window In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ==========
https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:35: RunningAverage(int steps_per_window); On 2017/05/12 16:06:26, Sami wrote: > Add explicit Done. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:36: int GetStepsPerWindow(); On 2017/05/12 16:06:26, Sami wrote: > Let's make this const Done. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator.h:38: base::TimeDelta GetAverage(); On 2017/05/12 16:06:26, Sami wrote: > Ditto. Done. https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:35: uint window_duration_step_ratio) On 2017/05/12 16:06:26, Sami wrote: > uint -> size_t (or just int) Good catch. Thanks! https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/queueing_time_estimator_unittest.cc:308: // | |\ On 2017/05/12 16:06:27, Sami wrote: > I love the diagrams! > > One thing to watch our for is that ending a // comment with a backslash means > the following line is commented too and I think Clang may warn about that > (http://stackoverflow.com/questions/30286253/how-to-escape-backslash-in-comment). > If the bots are fine with this then no need to worry but I'd just thought I'd > mention it. Thanks for mentioning that. Interesting point! https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2866613002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:555: base::TimeTicks uma_last_queueing_time_report_window_start_time_; On 2017/05/12 16:06:27, Sami wrote: > Please move this to the MainThreadOnly struct to make the threading constrains a > little more obvious. Done.
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2866613002/#ps120001 (title: "function const; move uma_variable in MainThreadOnly")
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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2866613002/#ps140001 (title: "rebase only")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2866613002/#ps160001 (title: "escape backslash")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skyostill@ You are right that we need to escape the back-slash at the end of comment line. CQ complained it when I tried to committed.
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 maxlg@chromium.org
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": 160001, "attempt_start_ts": 1494858928633810,
"parent_rev": "b95d16dd12e99c5f1c3ea2e7316fef8f50066f98", "commit_rev":
"7a8f8e51e1e0d43cb0a24a629d508bc473864a28"}
Message was sent while issue was closed.
Description was changed from ========== EQT: Change Expected Queuing Time from per-second to sliding window In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 ========== to ========== EQT: Change Expected Queuing Time from per-second to sliding window In order to record Sliding Window Expected Queuing Time(EQT) Metric in UMA, we need to extend the current EQT from the per-second approach to a sliding window approach. The change makes the original per-second use case a special case of the sliding window use cases. BUG=710449 Review-Url: https://codereview.chromium.org/2866613002 Cr-Commit-Position: refs/heads/master@{#471803} Committed: https://chromium.googlesource.com/chromium/src/+/7a8f8e51e1e0d43cb0a24a629d50... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7a8f8e51e1e0d43cb0a24a629d50... |
