Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
363003002:
Add duration estimation data to RenderingStats. (Closed)
|
Created:
6 years, 5 months ago by Dominik Grewe Modified:
6 years, 5 months ago CC:
chromium-reviews, cc-bugs_chromium.org, Sami, picksi1, alex clarke (OOO till 29th), Yufeng Shen (Slow to review) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd duration estimation data to RenderingStats.
The deadline scheduler in the Chrome compositor relies on runtime estimations of
various stages in the rendering pipeline. This patch adds the actual and
estimated runtimes of these stages to RenderingStats so they will be available
in tracing output. This will allow us to tune the estimators.
BUG=243459
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284428
Patch Set 1 #Patch Set 2 : Check we only add duration data once per frame. #
Total comments: 12
Patch Set 3 : Add ImplThreadRenderingStatsAccumulated #
Total comments: 7
Patch Set 4 : LTHI forwards estimates to rendering stats. #
Total comments: 4
Patch Set 5 : Rename classes and methods. #Patch Set 6 : ProxyTimingHistory adds samples. #
Total comments: 6
Patch Set 7 : Nest classes inside RenderingStats; add tests for TimeDeltaList #
Total comments: 4
Patch Set 8 : nits #Patch Set 9 : rebase #Patch Set 10 : add CC_EXPORT #
Messages
Total messages: 46 (0 generated)
I'm trying to dump the estimation data into the trace so we can use the data for tuning the estimators. The code currently triggers some of the DCHECKs. Please have a look at my comments and let me know what you think. https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:74: // There should only ever be one sample of these durations per frame. Adding up values doesn't really make sense. We could use a list instead. See my comment in RenderingStatsInstrumentation below. Afaict, this method is actually never called. So what's the use case for it? https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... File cc/debug/rendering_stats_instrumentation.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats_instrumentation.cc:150: DCHECK(impl_thread_rendering_stats_.draw_duration == base::TimeDelta()); I added these DCHECKS because I thought we'd only call this once per frame. It turns out they actually trigger though, so maybe I'm misunderstanding how this works. Or is this a bug? If we can't guarantee that we only get at most one call per frame, we could use a list of values instead (summing them up doesn't make sense here).
https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:61: "commit_to_activate_duration_estimate", Postfix labels with _ms so units are easy to figure out? https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:74: // There should only ever be one sample of these durations per frame. On 2014/07/02 14:43:22, Dominik Grewe wrote: > Adding up values doesn't really make sense. We could use a list instead. See my > comment in RenderingStatsInstrumentation below. > Yeah, it doesn't make sense. Definitely get rid of the logic below. Not a big deal, but maybe create a separate ImplThreadRenderingStatsAccumulated struct for purity? > Afaict, this method is actually never called. So what's the use case for it? I think it's used at the end of LTHI::DrawLayers with the call to AccumulateAndClearImplThreadStats. https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... File cc/debug/rendering_stats_instrumentation.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats_instrumentation.cc:150: DCHECK(impl_thread_rendering_stats_.draw_duration == base::TimeDelta()); On 2014/07/02 14:43:22, Dominik Grewe wrote: > I added these DCHECKS because I thought we'd only call this once per frame. It > turns out they actually trigger though, so maybe I'm misunderstanding how this > works. Or is this a bug? > > If we can't guarantee that we only get at most one call per frame, we could use > a list of values instead (summing them up doesn't make sense here). I think it has to do with the various draw early outs that might prevent the stats from resetting. ThreadProxy::DrawSwapInternal always reaches the final return, but LTHI::DrawLayers sometimes isn't even called by ThreadProxy::DrawSwapInternal or LayerTreeHostImpl::DrawLayers can return early if there is no damage. https://codereview.chromium.org/363003002/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1009: BeginMainFrameToCommitDurationEstimate(); Can you add a comment indicating why you are taking the estimate before adding the next sample to the timing history? I agree that this should be the approach, but it's not immediately obvious.
I've added a ImplThreadRenderingStatsAccumulated as a list of TimeDeltas. I initially considered making it templated, but I didn't see an easy way of turning that into a base::Value. Is this what you had in mind? https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:61: "commit_to_activate_duration_estimate", On 2014/07/02 22:39:28, brianderson wrote: > Postfix labels with _ms so units are easy to figure out? Done. https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... File cc/debug/rendering_stats_instrumentation.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats_instrumentation.cc:150: DCHECK(impl_thread_rendering_stats_.draw_duration == base::TimeDelta()); On 2014/07/02 22:39:29, brianderson wrote: > On 2014/07/02 14:43:22, Dominik Grewe wrote: > > I added these DCHECKS because I thought we'd only call this once per frame. It > > turns out they actually trigger though, so maybe I'm misunderstanding how this > > works. Or is this a bug? > > > > If we can't guarantee that we only get at most one call per frame, we could > use > > a list of values instead (summing them up doesn't make sense here). > > I think it has to do with the various draw early outs that might prevent the > stats from resetting. ThreadProxy::DrawSwapInternal always reaches the final > return, but LTHI::DrawLayers sometimes isn't even called by > ThreadProxy::DrawSwapInternal or LayerTreeHostImpl::DrawLayers can return early > if there is no damage. I see. Thanks for the clarification! https://codereview.chromium.org/363003002/diff/40001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1009: BeginMainFrameToCommitDurationEstimate(); On 2014/07/02 22:39:29, brianderson wrote: > Can you add a comment indicating why you are taking the estimate before adding > the next sample to the timing history? I agree that this should be the approach, > but it's not immediately obvious. Done here and below.
+cc london graphics team
https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats... cc/debug/rendering_stats.h:33: class CC_EXPORT ImplThreadRenderingStatsAccumulated { What's the plan for how to process/display/use the history of values?
https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats... cc/debug/rendering_stats.h:33: class CC_EXPORT ImplThreadRenderingStatsAccumulated { On 2014/07/07 20:01:42, brianderson wrote: > What's the plan for how to process/display/use the history of values? Potentially several things: 1) Add a metric to Telemetry to measure the error of estimates (seems to be quite noisy though, so not sure how useful this will be). 2) Make data visible in trace viewer similar to input latency diagrams. 3) For tuning of the estimators.
https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats... cc/debug/rendering_stats.h:33: class CC_EXPORT ImplThreadRenderingStatsAccumulated { On 2014/07/07 20:42:32, Dominik Grewe wrote: > On 2014/07/07 20:01:42, brianderson wrote: > > What's the plan for how to process/display/use the history of values? > > Potentially several things: > > 1) Add a metric to Telemetry to measure the error of estimates (seems to be > quite noisy though, so not sure how useful this will be). > How would we get the list of timestamps out? I know there are mechanisms to get per-frame stats out, but they don't appear to be implemented using rendering stats. Should this be using a different mechanism instead? > 2) Make data visible in trace viewer similar to input latency diagrams. How would it hook up to the trace viewer? Should we have trace events in the ProxyTimingHistory class instead? > > 3) For tuning of the estimators.
On 2014/07/07 21:43:14, brianderson wrote: > https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats.h > File cc/debug/rendering_stats.h (right): > > https://codereview.chromium.org/363003002/diff/60001/cc/debug/rendering_stats... > cc/debug/rendering_stats.h:33: class CC_EXPORT > ImplThreadRenderingStatsAccumulated { > On 2014/07/07 20:42:32, Dominik Grewe wrote: > > On 2014/07/07 20:01:42, brianderson wrote: > > > What's the plan for how to process/display/use the history of values? > > > > Potentially several things: > > > > 1) Add a metric to Telemetry to measure the error of estimates (seems to be > > quite noisy though, so not sure how useful this will be). > > > > How would we get the list of timestamps out? I know there are mechanisms to get > per-frame stats out, but they don't appear to be implemented using rendering > stats. Should this be using a different mechanism instead? Telemetry is using rendering stats to compute various metrics already: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > 2) Make data visible in trace viewer similar to input latency diagrams. > > How would it hook up to the trace viewer? Should we have trace events in the > ProxyTimingHistory class instead? The idea is that the trace viewer can read the data from the rendering stats trace events. So rather than creating more trace event, we just use the existing ones. The only disadvantage is that rendering stats need to be explicitly enabled (by setting --enable-gpu-benchmarking). > > > > > 3) For tuning of the estimators.
https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:61: "commit_to_activate_duration_estimate", On 2014/07/03 13:16:38, Dominik Grewe wrote: > On 2014/07/02 22:39:28, brianderson wrote: > > Postfix labels with _ms so units are easy to figure out? > > Done. I'd prefer consistent units for all time values. I know it's complicated (reference builds on perf bots!), but could we convert all time values in RenderingStats to milliseconds? https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:74: // There should only ever be one sample of these durations per frame. On 2014/07/02 22:39:28, brianderson wrote: > On 2014/07/02 14:43:22, Dominik Grewe wrote: > > Adding up values doesn't really make sense. We could use a list instead. See > my > > comment in RenderingStatsInstrumentation below. > > > > Yeah, it doesn't make sense. Definitely get rid of the logic below. Not a big > deal, but maybe create a separate ImplThreadRenderingStatsAccumulated struct for > purity? > > > Afaict, this method is actually never called. So what's the use case for it? > > I think it's used at the end of LTHI::DrawLayers with the call to > AccumulateAndClearImplThreadStats. > I think we can get rid of the accumulated stats entirely, because they are now used only to update the PaintTimeCounter in LayerTreeHostImpl, where they are converted back to per-frame stats. I believe this patch could be simplified considerably by first making this change.
https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... cc/debug/rendering_stats.cc:74: // There should only ever be one sample of these durations per frame. On 2014/07/08 15:01:36, ernstm wrote: > On 2014/07/02 22:39:28, brianderson wrote: > > On 2014/07/02 14:43:22, Dominik Grewe wrote: > > > Adding up values doesn't really make sense. We could use a list instead. See > > my > > > comment in RenderingStatsInstrumentation below. > > > > > > > Yeah, it doesn't make sense. Definitely get rid of the logic below. Not a big > > deal, but maybe create a separate ImplThreadRenderingStatsAccumulated struct > for > > purity? > > > > > Afaict, this method is actually never called. So what's the use case for it? > > > > I think it's used at the end of LTHI::DrawLayers with the call to > > AccumulateAndClearImplThreadStats. > > > > I think we can get rid of the accumulated stats entirely, because they are now > used only to update the PaintTimeCounter in LayerTreeHostImpl, where they are > converted back to per-frame stats. I believe this patch could be simplified > considerably by first making this change. > > I don't think it would simplify this patch a lot. If I understand correctly, we still need to use lists of values because we cannot guarantee that we don't call Add* twice on the same RenderingStats objects (see Brian's comment in RenderingStatsInstrumentation::AddDrawDuration).
On 2014/07/08 15:10:41, Dominik Grewe wrote: > https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc > File cc/debug/rendering_stats.cc (right): > > https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... > cc/debug/rendering_stats.cc:74: // There should only ever be one sample of these > durations per frame. > On 2014/07/08 15:01:36, ernstm wrote: > > On 2014/07/02 22:39:28, brianderson wrote: > > > On 2014/07/02 14:43:22, Dominik Grewe wrote: > > > > Adding up values doesn't really make sense. We could use a list instead. > See > > > my > > > > comment in RenderingStatsInstrumentation below. > > > > > > > > > > Yeah, it doesn't make sense. Definitely get rid of the logic below. Not a > big > > > deal, but maybe create a separate ImplThreadRenderingStatsAccumulated struct > > for > > > purity? > > > > > > > Afaict, this method is actually never called. So what's the use case for > it? > > > > > > I think it's used at the end of LTHI::DrawLayers with the call to > > > AccumulateAndClearImplThreadStats. > > > > > > > I think we can get rid of the accumulated stats entirely, because they are now > > used only to update the PaintTimeCounter in LayerTreeHostImpl, where they are > > converted back to per-frame stats. I believe this patch could be simplified > > considerably by first making this change. > > > > > > I don't think it would simplify this patch a lot. If I understand correctly, we > still need to use lists of values because we cannot guarantee that we don't call > Add* twice on the same RenderingStats objects (see Brian's comment in > RenderingStatsInstrumentation::AddDrawDuration). Do we want to record the values when we early out, or could we just keep the last one when we did draw?
On 2014/07/08 15:24:12, ernstm wrote: > On 2014/07/08 15:10:41, Dominik Grewe wrote: > > > https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats.cc > > File cc/debug/rendering_stats.cc (right): > > > > > https://codereview.chromium.org/363003002/diff/40001/cc/debug/rendering_stats... > > cc/debug/rendering_stats.cc:74: // There should only ever be one sample of > these > > durations per frame. > > On 2014/07/08 15:01:36, ernstm wrote: > > > On 2014/07/02 22:39:28, brianderson wrote: > > > > On 2014/07/02 14:43:22, Dominik Grewe wrote: > > > > > Adding up values doesn't really make sense. We could use a list instead. > > See > > > > my > > > > > comment in RenderingStatsInstrumentation below. > > > > > > > > > > > > > Yeah, it doesn't make sense. Definitely get rid of the logic below. Not a > > big > > > > deal, but maybe create a separate ImplThreadRenderingStatsAccumulated > struct > > > for > > > > purity? > > > > > > > > > Afaict, this method is actually never called. So what's the use case for > > it? > > > > > > > > I think it's used at the end of LTHI::DrawLayers with the call to > > > > AccumulateAndClearImplThreadStats. > > > > > > > > > > I think we can get rid of the accumulated stats entirely, because they are > now > > > used only to update the PaintTimeCounter in LayerTreeHostImpl, where they > are > > > converted back to per-frame stats. I believe this patch could be simplified > > > considerably by first making this change. > > > > > > > > > > I don't think it would simplify this patch a lot. If I understand correctly, > we > > still need to use lists of values because we cannot guarantee that we don't > call > > Add* twice on the same RenderingStats objects (see Brian's comment in > > RenderingStatsInstrumentation::AddDrawDuration). > > Do we want to record the values when we early out, or could we just keep the > last one when we did draw? Good questions. I think we should keep all values because that's what the estimator sees. It may be confusing if, for example, the estimation goes up but we never see the event that caused it to go up in the data. Once we include GPU latency we have to support lists of TimeDeltas any way, because we only get those values with some delay. So it's quite common to have multiple values per frame. Wdyt?
Brian, Manfred, do you have any more questions or comments?
I don't have the knowledge to understand how this will hook up to telemetry and trace viewer exactly, so I'd like someone else to review that aspect of the patch, but the rest lgtm.
https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1015: .layer_tree_host_impl->rendering_stats_instrumentation() I don't think ThreadProxy should be reaching through LTHI to the renderering stats instrumentation. Can we make these calls from LTHI methods?
https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1015: .layer_tree_host_impl->rendering_stats_instrumentation() On 2014/07/11 18:16:18, danakj wrote: > I don't think ThreadProxy should be reaching through LTHI to the renderering > stats instrumentation. Can we make these calls from LTHI methods? ThreadProxy could access RenderingStatsInstrumentation through LayerTreeHost. https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... cc/debug/rendering_stats.cc:39: ImplThreadRenderingStatsAccumulated::AsValueInMilliseconds() const { The method name is a bit misleading. The return value is not a single value in Milliseconds, but a list of such values. Maybe AsListValueInMilliseconds? https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:33: class CC_EXPORT ImplThreadRenderingStatsAccumulated { If we expect to use more of these list values in the future, then I would prefer a more generic name (they could be used in MainThreadRenderingStats as well). It is basically a list of TimeDeltas; Maybe call it TimeDeltaList?
Thanks for the comments. https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1015: .layer_tree_host_impl->rendering_stats_instrumentation() On 2014/07/14 13:12:38, ernstm wrote: > On 2014/07/11 18:16:18, danakj wrote: > > I don't think ThreadProxy should be reaching through LTHI to the renderering > > stats instrumentation. Can we make these calls from LTHI methods? > > ThreadProxy could access RenderingStatsInstrumentation through LayerTreeHost. As far as I understand, this method is always running on the compositor thread (see DCHECK above), so we can't access LayerTreeHost. I've added AddDrawDurationEstimate etc methods to LTHI so we can call into those methods from ThreadProxy without exposing rendering_stats_instrumentation. Dana, is this what you had in mind? https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... File cc/debug/rendering_stats.cc (right): https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... cc/debug/rendering_stats.cc:39: ImplThreadRenderingStatsAccumulated::AsValueInMilliseconds() const { On 2014/07/14 13:12:39, ernstm wrote: > The method name is a bit misleading. The return value is not a single value in > Milliseconds, but a list of such values. Maybe AsListValueInMilliseconds? Done. https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/100001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:33: class CC_EXPORT ImplThreadRenderingStatsAccumulated { On 2014/07/14 13:12:39, ernstm wrote: > If we expect to use more of these list values in the future, then I would prefer > a more generic name (they could be used in MainThreadRenderingStats as well). It > is basically a list of TimeDeltas; Maybe call it TimeDeltaList? Good point. Done.
https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/363003002/diff/60001/cc/trees/thread_proxy.cc... cc/trees/thread_proxy.cc:1015: .layer_tree_host_impl->rendering_stats_instrumentation() On 2014/07/14 13:37:52, Dominik Grewe wrote: > On 2014/07/14 13:12:38, ernstm wrote: > > On 2014/07/11 18:16:18, danakj wrote: > > > I don't think ThreadProxy should be reaching through LTHI to the renderering > > > stats instrumentation. Can we make these calls from LTHI methods? > > > > ThreadProxy could access RenderingStatsInstrumentation through LayerTreeHost. > > As far as I understand, this method is always running on the compositor thread > (see DCHECK above), so we can't access LayerTreeHost. > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call into those > methods from ThreadProxy without exposing rendering_stats_instrumentation. Dana, > is this what you had in mind? I liked the version that exposes rendering_stats_instrumentation on LTHI a bit better. rendering_stats_instrumentation is kind of global in cc already, because it needs to accumulate stats from many places (similar to tracing for all of Chrome). Dana, why would you prefer to not expose it on LTHI?
On Mon, Jul 14, 2014 at 10:05 AM, <ernstm@chromium.org> wrote: > > https://codereview.chromium.org/363003002/diff/60001/cc/ > trees/thread_proxy.cc > File cc/trees/thread_proxy.cc (right): > > https://codereview.chromium.org/363003002/diff/60001/cc/ > trees/thread_proxy.cc#newcode1015 > cc/trees/thread_proxy.cc:1015: > .layer_tree_host_impl->rendering_stats_instrumentation() > On 2014/07/14 13:37:52, Dominik Grewe wrote: > >> On 2014/07/14 13:12:38, ernstm wrote: >> > On 2014/07/11 18:16:18, danakj wrote: >> > > I don't think ThreadProxy should be reaching through LTHI to the >> > renderering > >> > > stats instrumentation. Can we make these calls from LTHI methods? >> > >> > ThreadProxy could access RenderingStatsInstrumentation through >> > LayerTreeHost. > > As far as I understand, this method is always running on the >> > compositor thread > >> (see DCHECK above), so we can't access LayerTreeHost. >> > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call >> > into those > >> methods from ThreadProxy without exposing >> > rendering_stats_instrumentation. Dana, > >> is this what you had in mind? >> > > I liked the version that exposes rendering_stats_instrumentation on LTHI > a bit better. rendering_stats_instrumentation is kind of global in cc > already, because it needs to accumulate stats from many places (similar > to tracing for all of Chrome). Dana, why would you prefer to not expose > it on LTHI? > Because it added a lot of LOC to thread proxy that has nothing to do with being threaded vs single thread. We'd have to duplicate it all in STP to get the same data there. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/14 15:01:02, danakj wrote: > On Mon, Jul 14, 2014 at 10:05 AM, <mailto:ernstm@chromium.org> wrote: > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > trees/thread_proxy.cc > > File cc/trees/thread_proxy.cc (right): > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > trees/thread_proxy.cc#newcode1015 > > cc/trees/thread_proxy.cc:1015: > > .layer_tree_host_impl->rendering_stats_instrumentation() > > On 2014/07/14 13:37:52, Dominik Grewe wrote: > > > >> On 2014/07/14 13:12:38, ernstm wrote: > >> > On 2014/07/11 18:16:18, danakj wrote: > >> > > I don't think ThreadProxy should be reaching through LTHI to the > >> > > renderering > > > >> > > stats instrumentation. Can we make these calls from LTHI methods? > >> > > >> > ThreadProxy could access RenderingStatsInstrumentation through > >> > > LayerTreeHost. > > > > As far as I understand, this method is always running on the > >> > > compositor thread > > > >> (see DCHECK above), so we can't access LayerTreeHost. > >> > > > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call > >> > > into those > > > >> methods from ThreadProxy without exposing > >> > > rendering_stats_instrumentation. Dana, > > > >> is this what you had in mind? > >> > > > > I liked the version that exposes rendering_stats_instrumentation on LTHI > > a bit better. rendering_stats_instrumentation is kind of global in cc > > already, because it needs to accumulate stats from many places (similar > > to tracing for all of Chrome). Dana, why would you prefer to not expose > > it on LTHI? > > > > Because it added a lot of LOC to thread proxy that has nothing to do with > being threaded vs single thread. We'd have to duplicate it all in STP to > get the same data there. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. But in the current implementation, ThreadProxy just calls the methods on LTHI that it would otherwise call on RenderingStatsImplementation, right?
On 2014/07/14 15:40:48, ernstm wrote: > On 2014/07/14 15:01:02, danakj wrote: > > On Mon, Jul 14, 2014 at 10:05 AM, <mailto:ernstm@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > trees/thread_proxy.cc > > > File cc/trees/thread_proxy.cc (right): > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > trees/thread_proxy.cc#newcode1015 > > > cc/trees/thread_proxy.cc:1015: > > > .layer_tree_host_impl->rendering_stats_instrumentation() > > > On 2014/07/14 13:37:52, Dominik Grewe wrote: > > > > > >> On 2014/07/14 13:12:38, ernstm wrote: > > >> > On 2014/07/11 18:16:18, danakj wrote: > > >> > > I don't think ThreadProxy should be reaching through LTHI to the > > >> > > > renderering > > > > > >> > > stats instrumentation. Can we make these calls from LTHI methods? > > >> > > > >> > ThreadProxy could access RenderingStatsInstrumentation through > > >> > > > LayerTreeHost. > > > > > > As far as I understand, this method is always running on the > > >> > > > compositor thread > > > > > >> (see DCHECK above), so we can't access LayerTreeHost. > > >> > > > > > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call > > >> > > > into those > > > > > >> methods from ThreadProxy without exposing > > >> > > > rendering_stats_instrumentation. Dana, > > > > > >> is this what you had in mind? > > >> > > > > > > I liked the version that exposes rendering_stats_instrumentation on LTHI > > > a bit better. rendering_stats_instrumentation is kind of global in cc > > > already, because it needs to accumulate stats from many places (similar > > > to tracing for all of Chrome). Dana, why would you prefer to not expose > > > it on LTHI? > > > > > > > Because it added a lot of LOC to thread proxy that has nothing to do with > > being threaded vs single thread. We'd have to duplicate it all in STP to > > get the same data there. > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > But in the current implementation, ThreadProxy just calls the methods on LTHI > that > it would otherwise call on RenderingStatsImplementation, right? Yeh, since ThreadProxy is our channel from Scheduler to LTHI that makes sense. SingleThreadProxy duplicates the calls that ThreadProxy makes to LTH/LTHI. I agree with this patch it's not a super strong argument, cuz stuff like the timing history is all done in ThreadProxy. IMO that stuff shouldn't be there either. Hopefully doing this via calls to LTHI at least keeps us putting a bit less not-thread-proxy-specific code into ThreadProxy, while also improving demeter. I just don't want us to keep adding anything to the ThreadProxy, because I'd like to see us move a lot of things out of ThreadProxy that aren't thread-specific in the future. This will get easier once SingleThreadProxy is a scheduler client as well, since then we'll have much more in common between the two modes with the only difference really being the presence of a thread. WDYT?
On 2014/07/14 15:55:52, danakj wrote: > On 2014/07/14 15:40:48, ernstm wrote: > > On 2014/07/14 15:01:02, danakj wrote: > > > On Mon, Jul 14, 2014 at 10:05 AM, <mailto:ernstm@chromium.org> wrote: > > > > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > trees/thread_proxy.cc > > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > trees/thread_proxy.cc#newcode1015 > > > > cc/trees/thread_proxy.cc:1015: > > > > .layer_tree_host_impl->rendering_stats_instrumentation() > > > > On 2014/07/14 13:37:52, Dominik Grewe wrote: > > > > > > > >> On 2014/07/14 13:12:38, ernstm wrote: > > > >> > On 2014/07/11 18:16:18, danakj wrote: > > > >> > > I don't think ThreadProxy should be reaching through LTHI to the > > > >> > > > > renderering > > > > > > > >> > > stats instrumentation. Can we make these calls from LTHI methods? > > > >> > > > > >> > ThreadProxy could access RenderingStatsInstrumentation through > > > >> > > > > LayerTreeHost. > > > > > > > > As far as I understand, this method is always running on the > > > >> > > > > compositor thread > > > > > > > >> (see DCHECK above), so we can't access LayerTreeHost. > > > >> > > > > > > > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call > > > >> > > > > into those > > > > > > > >> methods from ThreadProxy without exposing > > > >> > > > > rendering_stats_instrumentation. Dana, > > > > > > > >> is this what you had in mind? > > > >> > > > > > > > > I liked the version that exposes rendering_stats_instrumentation on LTHI > > > > a bit better. rendering_stats_instrumentation is kind of global in cc > > > > already, because it needs to accumulate stats from many places (similar > > > > to tracing for all of Chrome). Dana, why would you prefer to not expose > > > > it on LTHI? > > > > > > > > > > Because it added a lot of LOC to thread proxy that has nothing to do with > > > being threaded vs single thread. We'd have to duplicate it all in STP to > > > get the same data there. > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > But in the current implementation, ThreadProxy just calls the methods on LTHI > > that > > it would otherwise call on RenderingStatsImplementation, right? > > Yeh, since ThreadProxy is our channel from Scheduler to LTHI that makes sense. > SingleThreadProxy duplicates the calls that ThreadProxy makes to LTH/LTHI. > > I agree with this patch it's not a super strong argument, cuz stuff like the > timing history is all done in ThreadProxy. IMO that stuff shouldn't be there > either. Hopefully doing this via calls to LTHI at least keeps us putting a bit > less not-thread-proxy-specific code into ThreadProxy, while also improving > demeter. > > I just don't want us to keep adding anything to the ThreadProxy, because I'd > like to see us move a lot of things out of ThreadProxy that aren't > thread-specific in the future. This will get easier once SingleThreadProxy is a > scheduler client as well, since then we'll have much more in common between the > two modes with the only difference really being the presence of a thread. > > WDYT? I still don't see how adding a level of indirection through LTHI for those three calls on RenderingStatsInstrumentation improves the situation. We add 3 methods to LTHI that simply pass through the timing data to RSI. All the code in ThreadProxy is still there, it just calls LTHI->foo (which wraps RSI->foo) vs. LTHI->RSI->foo. RSI is our central perf logging facility in cc. It should be accessible from everywhere. If we don't want the timing code in ThreadProxy, then we should either refactor this CL or file a ticket to do so as a follow up.
On 2014/07/15 10:33:55, ernstm wrote: > On 2014/07/14 15:55:52, danakj wrote: > > On 2014/07/14 15:40:48, ernstm wrote: > > > On 2014/07/14 15:01:02, danakj wrote: > > > > On Mon, Jul 14, 2014 at 10:05 AM, <mailto:ernstm@chromium.org> wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > > trees/thread_proxy.cc > > > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > > trees/thread_proxy.cc#newcode1015 > > > > > cc/trees/thread_proxy.cc:1015: > > > > > .layer_tree_host_impl->rendering_stats_instrumentation() > > > > > On 2014/07/14 13:37:52, Dominik Grewe wrote: > > > > > > > > > >> On 2014/07/14 13:12:38, ernstm wrote: > > > > >> > On 2014/07/11 18:16:18, danakj wrote: > > > > >> > > I don't think ThreadProxy should be reaching through LTHI to the > > > > >> > > > > > renderering > > > > > > > > > >> > > stats instrumentation. Can we make these calls from LTHI methods? > > > > >> > > > > > >> > ThreadProxy could access RenderingStatsInstrumentation through > > > > >> > > > > > LayerTreeHost. > > > > > > > > > > As far as I understand, this method is always running on the > > > > >> > > > > > compositor thread > > > > > > > > > >> (see DCHECK above), so we can't access LayerTreeHost. > > > > >> > > > > > > > > > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call > > > > >> > > > > > into those > > > > > > > > > >> methods from ThreadProxy without exposing > > > > >> > > > > > rendering_stats_instrumentation. Dana, > > > > > > > > > >> is this what you had in mind? > > > > >> > > > > > > > > > > I liked the version that exposes rendering_stats_instrumentation on LTHI > > > > > a bit better. rendering_stats_instrumentation is kind of global in cc > > > > > already, because it needs to accumulate stats from many places (similar > > > > > to tracing for all of Chrome). Dana, why would you prefer to not expose > > > > > it on LTHI? > > > > > > > > > > > > > Because it added a lot of LOC to thread proxy that has nothing to do with > > > > being threaded vs single thread. We'd have to duplicate it all in STP to > > > > get the same data there. > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > > email > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > But in the current implementation, ThreadProxy just calls the methods on > LTHI > > > that > > > it would otherwise call on RenderingStatsImplementation, right? > > > > Yeh, since ThreadProxy is our channel from Scheduler to LTHI that makes sense. > > SingleThreadProxy duplicates the calls that ThreadProxy makes to LTH/LTHI. > > > > I agree with this patch it's not a super strong argument, cuz stuff like the > > timing history is all done in ThreadProxy. IMO that stuff shouldn't be there > > either. Hopefully doing this via calls to LTHI at least keeps us putting a bit > > less not-thread-proxy-specific code into ThreadProxy, while also improving > > demeter. > > > > I just don't want us to keep adding anything to the ThreadProxy, because I'd > > like to see us move a lot of things out of ThreadProxy that aren't > > thread-specific in the future. This will get easier once SingleThreadProxy is > a > > scheduler client as well, since then we'll have much more in common between > the > > two modes with the only difference really being the presence of a thread. > > > > WDYT? > > I still don't see how adding a level of indirection through LTHI for those three > calls > on RenderingStatsInstrumentation improves the situation. We add 3 methods to > LTHI that simply > pass through the timing data to RSI. All the code in ThreadProxy is still there, > it just calls > LTHI->foo (which wraps RSI->foo) vs. LTHI->RSI->foo. RSI is our central perf > logging > facility in cc. It should be accessible from everywhere. > > If we don't want the timing code in ThreadProxy, then we should either refactor > this CL > or file a ticket to do so as a follow up. What if we give the RSI to the timing_history and have it set these things when we say "DidCommit" etc instead of pulling the numbers out and passing them over to the RSI via threadproxy/lthi?
On 2014/07/15 15:07:07, danakj wrote: > On 2014/07/15 10:33:55, ernstm wrote: > > On 2014/07/14 15:55:52, danakj wrote: > > > On 2014/07/14 15:40:48, ernstm wrote: > > > > On 2014/07/14 15:01:02, danakj wrote: > > > > > On Mon, Jul 14, 2014 at 10:05 AM, <mailto:ernstm@chromium.org> wrote: > > > > > > > > > > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > > > trees/thread_proxy.cc > > > > > > File cc/trees/thread_proxy.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/363003002/diff/60001/cc/ > > > > > > trees/thread_proxy.cc#newcode1015 > > > > > > cc/trees/thread_proxy.cc:1015: > > > > > > .layer_tree_host_impl->rendering_stats_instrumentation() > > > > > > On 2014/07/14 13:37:52, Dominik Grewe wrote: > > > > > > > > > > > >> On 2014/07/14 13:12:38, ernstm wrote: > > > > > >> > On 2014/07/11 18:16:18, danakj wrote: > > > > > >> > > I don't think ThreadProxy should be reaching through LTHI to the > > > > > >> > > > > > > renderering > > > > > > > > > > > >> > > stats instrumentation. Can we make these calls from LTHI methods? > > > > > >> > > > > > > >> > ThreadProxy could access RenderingStatsInstrumentation through > > > > > >> > > > > > > LayerTreeHost. > > > > > > > > > > > > As far as I understand, this method is always running on the > > > > > >> > > > > > > compositor thread > > > > > > > > > > > >> (see DCHECK above), so we can't access LayerTreeHost. > > > > > >> > > > > > > > > > > > > I've added AddDrawDurationEstimate etc methods to LTHI so we can call > > > > > >> > > > > > > into those > > > > > > > > > > > >> methods from ThreadProxy without exposing > > > > > >> > > > > > > rendering_stats_instrumentation. Dana, > > > > > > > > > > > >> is this what you had in mind? > > > > > >> > > > > > > > > > > > > I liked the version that exposes rendering_stats_instrumentation on > LTHI > > > > > > a bit better. rendering_stats_instrumentation is kind of global in cc > > > > > > already, because it needs to accumulate stats from many places > (similar > > > > > > to tracing for all of Chrome). Dana, why would you prefer to not > expose > > > > > > it on LTHI? > > > > > > > > > > > > > > > > Because it added a lot of LOC to thread proxy that has nothing to do > with > > > > > being threaded vs single thread. We'd have to duplicate it all in STP to > > > > > get the same data there. > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send > an > > > > email > > > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > But in the current implementation, ThreadProxy just calls the methods on > > LTHI > > > > that > > > > it would otherwise call on RenderingStatsImplementation, right? > > > > > > Yeh, since ThreadProxy is our channel from Scheduler to LTHI that makes > sense. > > > SingleThreadProxy duplicates the calls that ThreadProxy makes to LTH/LTHI. > > > > > > I agree with this patch it's not a super strong argument, cuz stuff like the > > > timing history is all done in ThreadProxy. IMO that stuff shouldn't be there > > > either. Hopefully doing this via calls to LTHI at least keeps us putting a > bit > > > less not-thread-proxy-specific code into ThreadProxy, while also improving > > > demeter. > > > > > > I just don't want us to keep adding anything to the ThreadProxy, because I'd > > > like to see us move a lot of things out of ThreadProxy that aren't > > > thread-specific in the future. This will get easier once SingleThreadProxy > is > > a > > > scheduler client as well, since then we'll have much more in common between > > the > > > two modes with the only difference really being the presence of a thread. > > > > > > WDYT? > > > > I still don't see how adding a level of indirection through LTHI for those > three > > calls > > on RenderingStatsInstrumentation improves the situation. We add 3 methods to > > LTHI that simply > > pass through the timing data to RSI. All the code in ThreadProxy is still > there, > > it just calls > > LTHI->foo (which wraps RSI->foo) vs. LTHI->RSI->foo. RSI is our central perf > > logging > > facility in cc. It should be accessible from everywhere. > > > > If we don't want the timing code in ThreadProxy, then we should either > refactor > > this CL > > or file a ticket to do so as a follow up. > > What if we give the RSI to the timing_history and have it set these things when > we say "DidCommit" etc instead of pulling the numbers out and passing them over > to the RSI via threadproxy/lthi? Yes, let's do that.
ProxyTimingHistory now gets a RenderingStatsInstrumentation. All the logic for adding the data to the RSI has now moved to the timing history. Is that what you had in mind?
On 2014/07/17 13:24:55, Dominik Grewe wrote: > ProxyTimingHistory now gets a RenderingStatsInstrumentation. All the logic for > adding the data to the RSI has now moved to the timing history. Is that what you > had in mind? LGTM, but please wait for Dana to take a final look.
Thanks very much, this is much better. LGTM https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:34: class CC_EXPORT TimeDeltaList { can this be a tested class inside ImplThreadRenderingStats? Otherwise it should be in its own file as per style guide. https://codereview.chromium.org/363003002/diff/140001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:101: RenderingStatsInstrumentation*); variable name please
https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:34: class CC_EXPORT TimeDeltaList { On 2014/07/17 14:49:35, danakj wrote: > can this be a tested class inside ImplThreadRenderingStats? Otherwise it should > be in its own file as per style guide. This should not be inside ImplThreadRenderingStats, as it could potentially be used in MainThreadRenderingStats as well.
https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:34: class CC_EXPORT TimeDeltaList { On 2014/07/17 15:27:54, ernstm wrote: > On 2014/07/17 14:49:35, danakj wrote: > > can this be a tested class inside ImplThreadRenderingStats? Otherwise it > should > > be in its own file as per style guide. > > This should not be inside ImplThreadRenderingStats, as it could potentially be > used in MainThreadRenderingStats as well. Oh, though it's not currently. Technically this file already has 3 classes in it which is wrong. Maybe Main/Impl stats should be nested in RenderingStats. Then this could be nested there too at the same level and used by both. This is just style nits, so idk, but we should have 1 class per file.
https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stats.h File cc/debug/rendering_stats.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/debug/rendering_stat... cc/debug/rendering_stats.h:34: class CC_EXPORT TimeDeltaList { On 2014/07/17 15:42:59, danakj wrote: > On 2014/07/17 15:27:54, ernstm wrote: > > On 2014/07/17 14:49:35, danakj wrote: > > > can this be a tested class inside ImplThreadRenderingStats? Otherwise it > > should > > > be in its own file as per style guide. > > > > This should not be inside ImplThreadRenderingStats, as it could potentially be > > used in MainThreadRenderingStats as well. > > Oh, though it's not currently. > > Technically this file already has 3 classes in it which is wrong. Maybe > Main/Impl stats should be nested in RenderingStats. Then this could be nested > there too at the same level and used by both. > > This is just style nits, so idk, but we should have 1 class per file. I've nested the classes inside RenderingStats and added some simple unit tests for TimeDeltaList. Ptal. https://codereview.chromium.org/363003002/diff/140001/cc/trees/thread_proxy.h File cc/trees/thread_proxy.h (right): https://codereview.chromium.org/363003002/diff/140001/cc/trees/thread_proxy.h... cc/trees/thread_proxy.h:101: RenderingStatsInstrumentation*); On 2014/07/17 14:49:35, danakj wrote: > variable name please Done.
Thanks! Very cool. LGTM https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... File cc/debug/rendering_stats_unittest.cc (right): https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... cc/debug/rendering_stats_unittest.cc:26: } Expect_eq(0, getsize) too? https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... cc/debug/rendering_stats_unittest.cc:36: Expect_false(empty) too?
Thanks for the comments everyone! Looks much better now than before. https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... File cc/debug/rendering_stats_unittest.cc (right): https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... cc/debug/rendering_stats_unittest.cc:26: } On 2014/07/18 15:34:46, danakj wrote: > Expect_eq(0, getsize) too? Done. https://codereview.chromium.org/363003002/diff/180001/cc/debug/rendering_stat... cc/debug/rendering_stats_unittest.cc:36: On 2014/07/18 15:34:46, danakj wrote: > Expect_false(empty) too? Done.
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominikg@chromium.org/363003002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30423)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30450)
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominikg@chromium.org/363003002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was checked by dominikg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominikg@chromium.org/363003002/240001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 284428 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
