|
|
Description[scheduler] Add RendererScheduler.TaskDurationPerQueueType.
Complement RendererScheduler.NumberOfTasksPerQueueType metric by
reporting total duration of tasks split per queue type.
R=skyostil@chromium.org,isherman@chromium.org
CC=haraken@chromium.org
BUG=702314
Review-Url: https://codereview.chromium.org/2755953003
Cr-Commit-Position: refs/heads/master@{#472216}
Committed: https://chromium.googlesource.com/chromium/src/+/0bed265f220fcf4db86ad1356bed9ec274748b54
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Fix nit #
Total comments: 10
Patch Set 4 : Rebased & addressed comment #Patch Set 5 : rebased #
Total comments: 4
Patch Set 6 : addressed comments from skyostil@ and isherman@ #Patch Set 7 : fixed compilation #
Messages
Total messages: 45 (25 generated)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1796: duration.InMicroseconds(), 1, 1000000, 50); Optional nit: The constant 1000000 would be easier to read as "1000 * 1000", IMO. https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet("RendererScheduler.TaskDurationPerQueueType", 0, The min value should be 1, not 0. https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1812: static_cast<int>(TaskQueue::QueueType::COUNT), This should be COUNT+1 -- the final bucket should not be used. https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); It looks like histograms aggregate values in 32-bit counters. Did you somehow verify that overflow is not a concern? https://codereview.chromium.org/2755953003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56056: + and current accumulated duration is longer than 1ms. Please document that this histogram is not intended to be analyzed using the dashboard, but rather with manual scripts, because the dashboards cannot help to identify problematic outliers for this histogram structure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1796: duration.InMicroseconds(), 1, 1000000, 50); On 2017/03/16 21:23:43, Ilya Sherman wrote: > Optional nit: The constant 1000000 would be easier to read as "1000 * 1000", > IMO. Done. https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet("RendererScheduler.TaskDurationPerQueueType", 0, On 2017/03/16 21:23:43, Ilya Sherman wrote: > The min value should be 1, not 0. Done. https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1812: static_cast<int>(TaskQueue::QueueType::COUNT), On 2017/03/16 21:23:43, Ilya Sherman wrote: > This should be COUNT+1 -- the final bucket should not be used. Done. To be clear — if my samples are in range [0..N) I need to pass (1, N, N + 1) as parameters, right? https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/16 21:23:43, Ilya Sherman wrote: > It looks like histograms aggregate values in 32-bit counters. Did you somehow > verify that overflow is not a concern? By design we're expecting number of samples to be less than 1000 (samples/sec) * 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number overflows, we rely on UMA to throw the data out. https://codereview.chromium.org/2755953003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:56056: + and current accumulated duration is longer than 1ms. On 2017/03/16 21:23:43, Ilya Sherman wrote: > Please document that this histogram is not intended to be analyzed using the > dashboard, but rather with manual scripts, because the dashboards cannot help to > identify problematic outliers for this histogram structure. Done.
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1812: static_cast<int>(TaskQueue::QueueType::COUNT), On 2017/03/16 23:44:23, altimin wrote: > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > This should be COUNT+1 -- the final bucket should not be used. > > Done. To be clear — if my samples are in range [0..N) I need to pass (1, N, N + > 1) as parameters, right? Correct. Yeah, it's a fairly confusing API, sorry! https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/16 23:44:22, altimin wrote: > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > It looks like histograms aggregate values in 32-bit counters. Did you somehow > > verify that overflow is not a concern? > > By design we're expecting number of samples to be less than 1000 (samples/sec) * > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number overflows, > we rely on UMA to throw the data out. Metrics are not reset on each upload -- we simply take a snapshot, and compute differences. So if you're relying on the upload period being a reset of these metrics, then that won't work. Also, what do you mean by "we rely on UMA to throw the data out"? Where do you expect UMA to do that? https://codereview.chromium.org/2755953003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56059: + with custom scripts accounting for that instead for a dashboard. nit: s/instead for/rather than from
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/16 23:58:45, Ilya Sherman wrote: > On 2017/03/16 23:44:22, altimin wrote: > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > It looks like histograms aggregate values in 32-bit counters. Did you > somehow > > > verify that overflow is not a concern? > > > > By design we're expecting number of samples to be less than 1000 (samples/sec) > * > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > overflows, > > we rely on UMA to throw the data out. > > Metrics are not reset on each upload -- we simply take a snapshot, and compute > differences. So if you're relying on the upload period being a reset of these > metrics, then that won't work. > > Also, what do you mean by "we rely on UMA to throw the data out"? Where do you > expect UMA to do that? That's insightful, thanks. Any chance we could start using 64-bit integers here? By "throwing out" I meant that I was hoping that when overflow is detected, UMA will just throw data out.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1787: RecordTaskMetrics(task_queue->GetQueueType(), I'm just curious but wouldn't it be heavy to record histograms at every task?
https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1787: RecordTaskMetrics(task_queue->GetQueueType(), On 2017/03/17 11:59:51, haraken wrote: > > I'm just curious but wouldn't it be heavy to record histograms at every task? > We've started recording task lengths a while ago. This particular histogram is fine because we just increment a value and report only when value was sufficiently large (>1ms, but we might want to increase this threshold).
On 2017/03/17 00:24:00, altimin wrote: > https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... > File > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc > (right): > > https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: > static_cast<int>(milliseconds)); > On 2017/03/16 23:58:45, Ilya Sherman wrote: > > On 2017/03/16 23:44:22, altimin wrote: > > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > > It looks like histograms aggregate values in 32-bit counters. Did you > > somehow > > > > verify that overflow is not a concern? > > > > > > By design we're expecting number of samples to be less than 1000 > (samples/sec) > > * > > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > > overflows, > > > we rely on UMA to throw the data out. > > > > Metrics are not reset on each upload -- we simply take a snapshot, and compute > > differences. So if you're relying on the upload period being a reset of these > > metrics, then that won't work. > > > > Also, what do you mean by "we rely on UMA to throw the data out"? Where do > you > > expect UMA to do that? > > That's insightful, thanks. Any chance we could start using 64-bit integers here? > > By "throwing out" I meant that I was hoping that when overflow is detected, UMA > will just throw data out. Alternatively could we reset the counters ourselves (e.g., during navigations) or just stop reporting when we overflow? https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1798: UMA_HISTOGRAM_ENUMERATION("RendererScheduler.NumberOfTasksPerQueueType", Is this metric useful anymore with the new duration histogram? https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet( Could we cache this getter?
On 2017/03/17 12:53:39, Sami wrote: > On 2017/03/17 00:24:00, altimin wrote: > > > https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... > > File > > > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc > > (right): > > > > > https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... > > > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: > > static_cast<int>(milliseconds)); > > On 2017/03/16 23:58:45, Ilya Sherman wrote: > > > On 2017/03/16 23:44:22, altimin wrote: > > > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > > > It looks like histograms aggregate values in 32-bit counters. Did you > > > somehow > > > > > verify that overflow is not a concern? > > > > > > > > By design we're expecting number of samples to be less than 1000 > > (samples/sec) > > > * > > > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > > > overflows, > > > > we rely on UMA to throw the data out. > > > > > > Metrics are not reset on each upload -- we simply take a snapshot, and > compute > > > differences. So if you're relying on the upload period being a reset of > these > > > metrics, then that won't work. > > > > > > Also, what do you mean by "we rely on UMA to throw the data out"? Where do > > you > > > expect UMA to do that? > > > > That's insightful, thanks. Any chance we could start using 64-bit integers > here? > > > > By "throwing out" I meant that I was hoping that when overflow is detected, > UMA > > will just throw data out. > > Alternatively could we reset the counters ourselves (e.g., during navigations) > or just stop reporting when we overflow? We can't reset UMA counters, that's internals of base::Histogram. Stopping reporting when we overflow doesn't sound like a good idea — feels like this functionality should be in UMA and we are missing parts of the picture (if histogram was created before and already contains some samples, we won't know about it).
https://codereview.chromium.org/2755953003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:56059: + with custom scripts accounting for that instead for a dashboard. On 2017/03/16 23:58:45, Ilya Sherman wrote: > nit: s/instead for/rather than from Done. https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1798: UMA_HISTOGRAM_ENUMERATION("RendererScheduler.NumberOfTasksPerQueueType", On 2017/03/17 12:53:38, Sami wrote: > Is this metric useful anymore with the new duration histogram? Good question. Let's leave it for now (to see the relationship between the number of tasks and the total duration) and revisit it when we have the data. https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet( On 2017/03/17 12:53:38, Sami wrote: > Could we cache this getter? Let's avoid doing this — all macros do invoke Histogram::FactoryGet and it results in one hash map lookup.
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/17 00:24:00, altimin wrote: > On 2017/03/16 23:58:45, Ilya Sherman wrote: > > On 2017/03/16 23:44:22, altimin wrote: > > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > > It looks like histograms aggregate values in 32-bit counters. Did you > > somehow > > > > verify that overflow is not a concern? > > > > > > By design we're expecting number of samples to be less than 1000 > (samples/sec) > > * > > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > > overflows, > > > we rely on UMA to throw the data out. > > > > Metrics are not reset on each upload -- we simply take a snapshot, and compute > > differences. So if you're relying on the upload period being a reset of these > > metrics, then that won't work. > > > > Also, what do you mean by "we rely on UMA to throw the data out"? Where do > you > > expect UMA to do that? > > That's insightful, thanks. Any chance we could start using 64-bit integers here? I think we'd need to measure whether there are any adverse impacts on memory if we switch from 32-bit to 64-bit. If it's memory-neutral, then using 64-bit integers is probably reasonable. I don't think anyone on the metrics team is likely to get to this soon; but if you have time to drive such an effort, we could help advise you. > By "throwing out" I meant that I was hoping that when overflow is detected, UMA > will just throw data out. UMA doesn't do anything to detect overflow that I know of. (It's possible that we do and I don't know of it, though. Steve was mentioning something that sounded kind of like this on the email thread.) https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet( On 2017/03/17 13:05:01, altimin wrote: > On 2017/03/17 12:53:38, Sami wrote: > > Could we cache this getter? > > Let's avoid doing this — all macros do invoke Histogram::FactoryGet and it > results in one hash map lookup. Actually, the macros define a static pointer. That's pretty much the entire reason that we have macros rather than functions -- so as to cache the pointer.
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/17 20:27:36, Ilya Sherman wrote: > On 2017/03/17 00:24:00, altimin wrote: > > On 2017/03/16 23:58:45, Ilya Sherman wrote: > > > On 2017/03/16 23:44:22, altimin wrote: > > > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > > > It looks like histograms aggregate values in 32-bit counters. Did you > > > somehow > > > > > verify that overflow is not a concern? > > > > > > > > By design we're expecting number of samples to be less than 1000 > > (samples/sec) > > > * > > > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > > > overflows, > > > > we rely on UMA to throw the data out. > > > > > > Metrics are not reset on each upload -- we simply take a snapshot, and > compute > > > differences. So if you're relying on the upload period being a reset of > these > > > metrics, then that won't work. > > > > > > Also, what do you mean by "we rely on UMA to throw the data out"? Where do > > you > > > expect UMA to do that? > > > > That's insightful, thanks. Any chance we could start using 64-bit integers > here? > > I think we'd need to measure whether there are any adverse impacts on memory if > we switch from 32-bit to 64-bit. If it's memory-neutral, then using 64-bit > integers is probably reasonable. I don't think anyone on the metrics team is > likely to get to this soon; but if you have time to drive such an effort, we > could help advise you. > > > By "throwing out" I meant that I was hoping that when overflow is detected, > UMA > > will just throw data out. > > UMA doesn't do anything to detect overflow that I know of. (It's possible that > we do and I don't know of it, though. Steve was mentioning something that > sounded kind of like this on the email thread.) I took a quick look at this — it seems that absence of 64-bit atomic integers at some platforms can be a problem too. So I'd like to suggest a special 64-bit subtype of histogram (templates to the rescue?) with a comment that it can cause performance and memory overhead. WDYT?
https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1815: static_cast<int>(milliseconds)); On 2017/03/17 23:49:53, altimin wrote: > On 2017/03/17 20:27:36, Ilya Sherman wrote: > > On 2017/03/17 00:24:00, altimin wrote: > > > On 2017/03/16 23:58:45, Ilya Sherman wrote: > > > > On 2017/03/16 23:44:22, altimin wrote: > > > > > On 2017/03/16 21:23:43, Ilya Sherman wrote: > > > > > > It looks like histograms aggregate values in 32-bit counters. Did you > > > > somehow > > > > > > verify that overflow is not a concern? > > > > > > > > > > By design we're expecting number of samples to be less than 1000 > > > (samples/sec) > > > > * > > > > > 1800 (sec / upload) * 100 (renderers) ~ 10^8 samples. If this number > > > > overflows, > > > > > we rely on UMA to throw the data out. > > > > > > > > Metrics are not reset on each upload -- we simply take a snapshot, and > > compute > > > > differences. So if you're relying on the upload period being a reset of > > these > > > > metrics, then that won't work. > > > > > > > > Also, what do you mean by "we rely on UMA to throw the data out"? Where > do > > > you > > > > expect UMA to do that? > > > > > > That's insightful, thanks. Any chance we could start using 64-bit integers > > here? > > > > I think we'd need to measure whether there are any adverse impacts on memory > if > > we switch from 32-bit to 64-bit. If it's memory-neutral, then using 64-bit > > integers is probably reasonable. I don't think anyone on the metrics team is > > likely to get to this soon; but if you have time to drive such an effort, we > > could help advise you. > > > > > By "throwing out" I meant that I was hoping that when overflow is detected, > > UMA > > > will just throw data out. > > > > UMA doesn't do anything to detect overflow that I know of. (It's possible > that > > we do and I don't know of it, though. Steve was mentioning something that > > sounded kind of like this on the email thread.) > > I took a quick look at this — it seems that absence of 64-bit atomic integers at > some platforms can be a problem too. > > So I'd like to suggest a special 64-bit subtype of histogram (templates to the > rescue?) with a comment that it can cause performance and memory overhead. WDYT? Hmm, I guess it depends on how much of a maintenance cost it adds. I'm a bit hesitant to introduce yet another type of histogram that is slightly different than others, especially given that the current known use-case requires custom analysis scripts to work with it. I'd suggest perhaps bringing this up on the metrics team mailing list. I'd still lean toward a solution that uses one histogram per queue type, fwiw.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2867303004/ has landed, PTAL
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) 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 altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Metrics LGTM, thanks. https://codereview.chromium.org/2755953003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1867: base::TimeDelta& total_duration = Optional nit: Maybe name this something like "unreported_duration" for greater clarity? https://codereview.chromium.org/2755953003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58922: + usage of each type of task queues. Reported each time when task is completed nit: s/queues/queue
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1798: UMA_HISTOGRAM_ENUMERATION("RendererScheduler.NumberOfTasksPerQueueType", On 2017/03/17 13:05:01, altimin wrote: > On 2017/03/17 12:53:38, Sami wrote: > > Is this metric useful anymore with the new duration histogram? > > Good question. Let's leave it for now (to see the relationship between the > number of tasks and the total duration) and revisit it when we have the data. Ok, let's add a TODO to look into it though?
https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1798: UMA_HISTOGRAM_ENUMERATION("RendererScheduler.NumberOfTasksPerQueueType", On 2017/05/16 10:45:07, Sami wrote: > On 2017/03/17 13:05:01, altimin wrote: > > On 2017/03/17 12:53:38, Sami wrote: > > > Is this metric useful anymore with the new duration histogram? > > > > Good question. Let's leave it for now (to see the relationship between the > > number of tasks and the total duration) and revisit it when we have the data. > > Ok, let's add a TODO to look into it though? Done. https://codereview.chromium.org/2755953003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1810: base::Histogram::FactoryGet( On 2017/03/17 20:27:36, Ilya Sherman wrote: > On 2017/03/17 13:05:01, altimin wrote: > > On 2017/03/17 12:53:38, Sami wrote: > > > Could we cache this getter? > > > > Let's avoid doing this — all macros do invoke Histogram::FactoryGet and it > > results in one hash map lookup. > > Actually, the macros define a static pointer. That's pretty much the entire > reason that we have macros rather than functions -- so as to cache the pointer. Done. https://codereview.chromium.org/2755953003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2755953003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1867: base::TimeDelta& total_duration = On 2017/05/15 22:53:22, Ilya Sherman wrote: > Optional nit: Maybe name this something like "unreported_duration" for greater > clarity? Done. https://codereview.chromium.org/2755953003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2755953003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:58922: + usage of each type of task queues. Reported each time when task is completed On 2017/05/15 22:53:22, Ilya Sherman wrote: > nit: s/queues/queue Done.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2755953003/#ps120001 (title: "fixed compilation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494941858394820, "parent_rev": "23cd806334edf349b4371e63f231bc1361fe0a08", "commit_rev": "0bed265f220fcf4db86ad1356bed9ec274748b54"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Add RendererScheduler.TaskDurationPerQueueType. Complement RendererScheduler.NumberOfTasksPerQueueType metric by reporting total duration of tasks split per queue type. R=skyostil@chromium.org,isherman@chromium.org CC=haraken@chromium.org BUG=702314 ========== to ========== [scheduler] Add RendererScheduler.TaskDurationPerQueueType. Complement RendererScheduler.NumberOfTasksPerQueueType metric by reporting total duration of tasks split per queue type. R=skyostil@chromium.org,isherman@chromium.org CC=haraken@chromium.org BUG=702314 Review-Url: https://codereview.chromium.org/2755953003 Cr-Commit-Position: refs/heads/master@{#472216} Committed: https://chromium.googlesource.com/chromium/src/+/0bed265f220fcf4db86ad1356bed... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0bed265f220fcf4db86ad1356bed... |