Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2890173002:
EQT: Record the max queueing time from navigation start to navigation away (Closed)
|
Created:
3 years, 7 months ago by Liquan (Max) Gu Modified:
3 years, 6 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch, scheduler-bugs_chromium.org, asvitkine+watch_chromium.org, nasko, clamy, alex clarke (OOO till 29th) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEQT: Record the max queueing time from navigation start to navigation away
We want to record the maximum EQT in an N ms sliding window between TTI and when
the user navigates away, yet TTI is not landed yet, so this CL will
measure the max EQT from navigation start to navigation away as a first step.
BUG=710449
Review-Url: https://codereview.chromium.org/2890173002
Cr-Commit-Position: refs/heads/master@{#477289}
Committed: https://chromium.googlesource.com/chromium/src/+/206f88a3c6df64c70511446235ed6ff72a94ce9e
Patch Set 1 #Patch Set 2 : add DEP #
Total comments: 10
Patch Set 3 : record max; flush hist; #
Total comments: 6
Patch Set 4 : flush hist in OnCommitProvitionalLoad #
Total comments: 9
Patch Set 5 : rebase #Patch Set 6 : add a single thread and multi-thread test #
Total comments: 14
Patch Set 7 : test cover the max; test cover during nav #
Total comments: 4
Patch Set 8 : nit: GetOrCreate to Create #Patch Set 9 : rebase #Patch Set 10 : fix duplicate creations of the metric #Patch Set 11 : DEPS adjust #
Total comments: 10
Patch Set 12 : comments and todo #Patch Set 13 : fix test nits #Patch Set 14 : fix mock_renderer_scheduler #Messages
Total messages: 79 (34 generated)
Description was changed from ========== add DEP maxEQT BUG= ========== to ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will do it from navigation start to navigation away temporarily. BUG=710449 ==========
maxlg@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will do it from navigation start to navigation away temporarily. BUG=710449 ========== to ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will do it from navigation start to navigation away temporarily. BUG=710449 ==========
Description was changed from ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will do it from navigation start to navigation away temporarily. BUG=710449 ========== to ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will measure the max EQT from navigation start to navigation away as a first step. BUG=710449 ==========
We've historically been pretty bad about testing histograms, but we're trying to get better. Use https://cs.chromium.org/chromium/src/base/test/histogram_tester.h?q=histogram.... Once you've added the flush logic, you can test the case where the renderer isn't torn down between navigations by calling |scheduler_->OnNavigationStarted()| in your test. Then to test the case where the renderer is torn down, I think you just to |scheduler_.reset()| in your test. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right): https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/DEPS:4: "+components/metrics", We should make sure to get components/metrics OWNER to review this. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:222: "RendererScheduler.MaxQueueingTime", This needs to line up with the name in histograms.xml. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1676: void RendererSchedulerImpl::ResetForNavigationLocked() { We should flush the histogram in here. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1914: queueing_time.ToInternalValue()); We should be using InMilliseconds(), I think. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1931: Try to avoid introducing unrelated whitespace changes.
I have experiemented on resetting the single sample histogram on ResetForNavigationLocked()/OnNavigationStarted(). I found that each refreshing of the page actually triggers two OnNavigationStarted()/OnNavigationStarted(), which makes the histogram record two values by each refresh. Intuitively there should be only one. I found one of the two callings coming from render_frame_impl.DidCommitProvisionalLoad(). https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right): https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/DEPS:4: "+components/metrics", On 2017/05/18 20:42:26, tdresser wrote: > We should make sure to get components/metrics OWNER to review this. Acknowledged. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:222: "RendererScheduler.MaxQueueingTime", On 2017/05/18 20:42:26, tdresser wrote: > This needs to line up with the name in histograms.xml. Done. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1676: void RendererSchedulerImpl::ResetForNavigationLocked() { On 2017/05/18 20:42:26, tdresser wrote: > We should flush the histogram in here. Done. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1914: queueing_time.ToInternalValue()); On 2017/05/18 20:42:26, tdresser wrote: > We should be using InMilliseconds(), I think. Done. https://codereview.chromium.org/2890173002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1931: On 2017/05/18 20:42:26, tdresser wrote: > Try to avoid introducing unrelated whitespace changes. Done.
+creis@, any thoughts on what signal we should be using for a page navigation starting? "I found that each refreshing of the page actually triggers two OnNavigationStarted()/OnNavigationStarted(), which makes the histogram record two values by each refresh. Intuitively there should be only one. I found one of the two callings coming from render_frame_impl.DidCommitProvisionalLoad()." https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1913: if (!GetMainThreadOnly().max_queuing_time_histogram) { Add a comment about why we're instantiating this here. https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:16: #include "components/metrics/single_sample_metrics_factory_impl.h" You can include the factory in the cc file, right? I don't think it's needed here. https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:497: bool during_navigation; Add a comment about what it means to be "during_navigation". The purpose of this attribute isn't clear to me. It looks like it's set to true on the first OnNavigationStarted, and then is never set to false again?
creis@chromium.org changed reviewers: + creis@chromium.org
On 2017/05/23 14:57:52, tdresser wrote: > +creis@, any thoughts on what signal we should be using for a page navigation > starting? > > "I found that each refreshing of the page actually triggers two > OnNavigationStarted()/OnNavigationStarted(), which makes the histogram record > two values by each refresh. Intuitively there should be only one. I found one of > the two callings coming from render_frame_impl.DidCommitProvisionalLoad()." Hmm, I'm pretty confused about the context here. What are EQT and TTI? What types of navigations are you looking at? Just from a quick glance at the call sites of RendererSchedulerImpl::OnNavigationStarted, I don't see how that code can work. It appears to be broken for PlzNavigate, since it's called from RenderFrameImpl::OnNavigate, which is code that isn't used in PlzNavigate. It's also called in some cases from RenderFrameImpl::DidCommitProvisionalLoad, which is the *end* of a navigation, not the start, and might explain why you're seeing it twice in some places. I'm not familiar with RendererSchedulerImpl, but that seems pretty problematic to me. Even the idea of tracking navigation start time from the renderer process seems flawed, since renderers won't hear about most navigations until commit time once PlzNavigate ships. Maybe we should have a higher level discussion about how this code is supposed to work, along with Nasko and Camille?
+Sami as FYI that RendererSchedulerImpl is pretty broken.
On 2017/05/23 19:33:38, tdresser wrote: > +Sami as FYI that RendererSchedulerImpl is pretty broken. Following up with the scheduler team on the higher level issue sounds worth doing. I'm not certain that this patch needs to block on this though. Some context: EQT is the "Expected Queueing Time", a measure of main thread business. TTI is the "Time to Interactive" a measure of when a page becomes interactive. We want to measure how busy the main thread is, after we've deemed the page interactive. I think we can probably get this specific use case to work without sorting out all the issues in RendererSchedulerImpl. In the long term, we'll be using TTI as the start point, so we can fudge the start point for now - it sounds like using DidCommitProvisionalLoad would be an acceptable start point. In terms of an end point, I think we might be able to get away with DidCommitProvisionalLoad here too, though I'm not sure. The single sample metrics provider handles the fast shutdown case behind the scenes, so we only really need to handle the case where the main thread navigated away. We don't really care when the navigation really started, just when the main thread started processing it. It sounds like in the PlzNavigate world, that should approximately be DidCommitProvisionalLoad? Does that sound reasonable?
On 2017/05/23 19:09:34, Charlie Reis wrote: > Just from a quick glance at the call sites of > RendererSchedulerImpl::OnNavigationStarted, I don't see how that code can work. > It appears to be broken for PlzNavigate, since it's called from > RenderFrameImpl::OnNavigate, which is code that isn't used in PlzNavigate. It's > also called in some cases from RenderFrameImpl::DidCommitProvisionalLoad, which > is the *end* of a navigation, not the start, and might explain why you're seeing > it twice in some places. It was written before PlzNavigate which might explain some of the breakage :) > I'm not familiar with RendererSchedulerImpl, but that seems pretty problematic > to me. Even the idea of tracking navigation start time from the renderer > process seems flawed, since renderers won't hear about most navigations until > commit time once PlzNavigate ships. > > Maybe we should have a higher level discussion about how this code is supposed > to work, along with Nasko and Camille? Happy to chat more, but basically we need a signal telling when the current renderer starts loading a new page to a) reset some internal tracking metrics and 2) set more appropriate task scheduling policies while we're loading. Could you point us to a more reliable signal for this?
Sorry for the delay! Meetings yesterday and stability sheriffing today kept me busy. On 2017/05/24 17:24:27, Sami wrote: > On 2017/05/23 19:09:34, Charlie Reis wrote: > > Just from a quick glance at the call sites of > > RendererSchedulerImpl::OnNavigationStarted, I don't see how that code can > work. > > It appears to be broken for PlzNavigate, since it's called from > > RenderFrameImpl::OnNavigate, which is code that isn't used in PlzNavigate. > It's > > also called in some cases from RenderFrameImpl::DidCommitProvisionalLoad, > which > > is the *end* of a navigation, not the start, and might explain why you're > seeing > > it twice in some places. > > It was written before PlzNavigate which might explain some of the breakage :) > > > I'm not familiar with RendererSchedulerImpl, but that seems pretty problematic > > to me. Even the idea of tracking navigation start time from the renderer > > process seems flawed, since renderers won't hear about most navigations until > > commit time once PlzNavigate ships. > > > > Maybe we should have a higher level discussion about how this code is supposed > > to work, along with Nasko and Camille? > > Happy to chat more, but basically we need a signal telling when the current > renderer starts loading a new page to a) reset some internal tracking metrics > and 2) set more appropriate task scheduling policies while we're loading. Could > you point us to a more reliable signal for this? Sure. When you say "starts loading" here, I do tend to agree with tdresser@ that DidCommitProvisionalLoad is the point you want to consider, since that's when the renderer stops doing work on the old page and starts doing work on the new page. At any earlier point in time, the renderer process is still painting and running scripts (etc) on the old page, and it has only done minimal work to fire off some network requests for the new page (non-PlzNavigate only). In PlzNavigate, the renderer may not even know about the new navigation until CommitNavigation / DidCommitProvisionalLoad.
On 2017/05/25 02:03:00, Charlie Reis (overloaded) wrote: > Sure. When you say "starts loading" here, I do tend to agree with tdresser@ > that DidCommitProvisionalLoad is the point you want to consider, since that's > when the renderer stops doing work on the old page and starts doing work on the > new page. At any earlier point in time, the renderer process is still painting > and running scripts (etc) on the old page, and it has only done minimal work to > fire off some network requests for the new page (non-PlzNavigate only). In > PlzNavigate, the renderer may not even know about the new navigation until > CommitNavigation / DidCommitProvisionalLoad. Thanks! Filed https://bugs.chromium.org/p/chromium/issues/detail?id=726341 to track this.
Thanks for Saml, Charlie, and Tim. I use OnCommitProvisionalLoad as navigation start now. I've done some experiments and found OnNavigation is not fired when I click "Image" on www.google.com. And OnCommitProvisionalLoad is fired at each navigation start. https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1913: if (!GetMainThreadOnly().max_queuing_time_histogram) { On 2017/05/23 14:57:51, tdresser wrote: > Add a comment about why we're instantiating this here. Done. https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:16: #include "components/metrics/single_sample_metrics_factory_impl.h" On 2017/05/23 14:57:51, tdresser wrote: > You can include the factory in the cc file, right? I don't think it's needed > here. Done. We don't have to include factory. I include only the metric instead in my patch. https://codereview.chromium.org/2890173002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:497: bool during_navigation; On 2017/05/23 14:57:51, tdresser wrote: > Add a comment about what it means to be "during_navigation". > > The purpose of this attribute isn't clear to me. It looks like it's set to true > on the first OnNavigationStarted, and then is never set to false again? Yes, we want to use this attribute to exclude those EQT before the navigation, but this attribute includes EQT between the first navigation end and the next navigation start that happens at the same thread, not sure whether it sounds good to you?
https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:3824: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); Do you think we should combine the part in OnNavigationStarted() into OnCommitProvisionalLoad(), so that here only needs OnCommitProvisionalLoad(). https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/DEPS:4: "+base/metrics", Not sure whether it's OK to include base/metrics here. https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:500: std::unique_ptr<base::SingleSampleMetric> max_queueing_time_histogram; Is it better to call max_queueing_time_metric instead? A single value cannot be called histogram after all.
This is starting to look pretty reasonable. Does this work in manual testing, including when the renderer dies? Next up is some automated tests. https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:3824: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > Do you think we should combine the part in OnNavigationStarted() into > OnCommitProvisionalLoad(), so that here only needs OnCommitProvisionalLoad(). I propose we leave it for now, and as a follow up (potentially done by scheduler folks), we move remove OnNavigationStarted completely, and do all associated work in OnCommitProvisionalLoad. https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/DEPS:4: "+base/metrics", On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > Not sure whether it's OK to include base/metrics here. Looks like we're being careful here. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/DEPS?... Let's explicitly just add the file we need. https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:500: std::unique_ptr<base::SingleSampleMetric> max_queueing_time_histogram; On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > Is it better to call max_queueing_time_metric instead? A single value cannot be > called histogram after all. Yup, I agree that's a better name.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:3824: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); On 2017/05/25 19:43:38, tdresser wrote: > On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > > Do you think we should combine the part in OnNavigationStarted() into > > OnCommitProvisionalLoad(), so that here only needs OnCommitProvisionalLoad(). > > I propose we leave it for now, and as a follow up (potentially done by scheduler > folks), we move remove OnNavigationStarted completely, and do all associated > work in OnCommitProvisionalLoad. Acknowledged. https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/DEPS (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/DEPS:4: "+base/metrics", On 2017/05/25 19:43:38, tdresser wrote: > On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > > Not sure whether it's OK to include base/metrics here. > > Looks like we're being careful here. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/DEPS?... > > Let's explicitly just add the file we need. Done. https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h (right): https://codereview.chromium.org/2890173002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:500: std::unique_ptr<base::SingleSampleMetric> max_queueing_time_histogram; On 2017/05/25 19:43:38, tdresser wrote: > On 2017/05/25 16:34:40, Liquan (Max) Gu wrote: > > Is it better to call max_queueing_time_metric instead? A single value cannot > be > > called histogram after all. > > Yup, I agree that's a better name. Done.
tdresser@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil for scheduler review. Looks great, thanks. Let's expand the test coverage a bit - we never have a case where there's a high EQT followed by a low EQT, demonstrating that we are in fact gathering the maximum. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1637: // Initialize |max_queueing_time_metric| as we need it so that I think "as we need it" -> "lazily" is a bit clearer. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1642: } Do we need this? We reset it immediately below. Don't we only need the lazy initialization in OnQueueingTimeForWindowEstimated? https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1930: if (GetMainThreadOnly().has_navigated) { Can we get here when |has_navigated| is false? If not, we can get rid of has_navigated. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3952: // ignored because max queueing time is recorded after navigation start is recorded -> is only recorded https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3972: TEST_F(RendererSchedulerImplTest, MaxQueueingTimeMetricInTwoThreads) { We're only ever going to be adding EQT information from a single thread, aren't we? When we were talking about a test with threading, I think the case we were talking about was where you want to kill the thread that the Renderer Scheduler lives on, to simulate the hard shut down case. If we've tested that case manually, it's probably fine to rely on the testing of the SingleSampleMetric to verify that works. https://codereview.chromium.org/2890173002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2890173002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59368: + navigation and user navigate away. Mention the way the window works, and the current window size.
https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1649: RendererSchedulerImpl::GetMaxQueueingTimeMetric() { GetOrCreate
https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1637: // Initialize |max_queueing_time_metric| as we need it so that On 2017/05/30 13:34:44, tdresser wrote: > I think "as we need it" -> "lazily" is a bit clearer. Done. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1642: } On 2017/05/30 13:34:44, tdresser wrote: > Do we need this? We reset it immediately below. > Don't we only need the lazy initialization in OnQueueingTimeForWindowEstimated? Lazy init is not happened just from the beginning. Every time reset happen, it needs to get the metric again. Let's say NS is OnCommitProvisionalLoad, Q = OnQueueingTimeForWindowEstimated. After resetting in NS, we need lazy init again, but there is chances for both NS and Q to happen next, So that's why we need that in both places. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1649: RendererSchedulerImpl::GetMaxQueueingTimeMetric() { On 2017/05/30 15:44:41, tdresser wrote: > GetOrCreate Done. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1930: if (GetMainThreadOnly().has_navigated) { On 2017/05/30 13:34:44, tdresser wrote: > Can we get here when |has_navigated| is false? > If not, we can get rid of has_navigated. Yes, the EQTEstimator starts to measure EQTs before navigation start. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3952: // ignored because max queueing time is recorded after navigation start On 2017/05/30 13:34:44, tdresser wrote: > is recorded -> is only recorded Done. https://codereview.chromium.org/2890173002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3972: TEST_F(RendererSchedulerImplTest, MaxQueueingTimeMetricInTwoThreads) { On 2017/05/30 13:34:44, tdresser wrote: > We're only ever going to be adding EQT information from a single thread, aren't > we? > > When we were talking about a test with threading, I think the case we were > talking about was where you want to kill the thread that the Renderer Scheduler > lives on, to simulate the hard shut down case. > > If we've tested that case manually, it's probably fine to rely on the testing of > the SingleSampleMetric to verify that works. Talked offline with Tim, as SingleSampleMetric already covered multithread, we don't need to duplicate it here. https://codereview.chromium.org/2890173002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2890173002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:59368: + navigation and user navigate away. On 2017/05/30 13:34:44, tdresser wrote: > Mention the way the window works, and the current window size. Done.
https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1651: return base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric( Based on the method name I would expect this to cache the result in max_queueing_time_metric. Or do we need the second level cache at all if the metrics factory handles everything for us?
https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1651: return base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric( On 2017/05/31 10:30:34, Sami wrote: > Based on the method name I would expect this to cache the result in > max_queueing_time_metric. Or do we need the second level cache at all if the > metrics factory handles everything for us? There is a difference between caching it in max_queueing_time_metric or not. If not caching, the single sample metric will destruct every time when we create or get the metric to set value or reset the pointer. If caching, the metric won't destruct when we want to set value only, and this is a necessary property for the implementation. That's why we need this caching. The singe sample metric is designed in a way that it inserts a sample to the histogram when it destructs, so destruction should be used only when necessary.
https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1651: return base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric( On 2017/06/01 14:39:39, Liquan (Max) Gu wrote: > On 2017/05/31 10:30:34, Sami wrote: > > Based on the method name I would expect this to cache the result in > > max_queueing_time_metric. Or do we need the second level cache at all if the > > metrics factory handles everything for us? > > There is a difference between caching it in max_queueing_time_metric or not. If > not caching, the single sample metric will destruct every time when we create or > get the metric to set value or reset the pointer. If caching, the metric won't > destruct when we want to set value only, and this is a necessary property for > the implementation. That's why we need this caching. > > The singe sample metric is designed in a way that it inserts a sample to the > histogram when it destructs, so destruction should be used only when necessary. I see. This function seems to create a new metric every time it's called so I think the name GetOrCreate... is a little confusing, right?
https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1651: return base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric( On 2017/06/01 18:29:40, Sami wrote: > On 2017/06/01 14:39:39, Liquan (Max) Gu wrote: > > On 2017/05/31 10:30:34, Sami wrote: > > > Based on the method name I would expect this to cache the result in > > > max_queueing_time_metric. Or do we need the second level cache at all if the > > > metrics factory handles everything for us? > > > > There is a difference between caching it in max_queueing_time_metric or not. > If > > not caching, the single sample metric will destruct every time when we create > or > > get the metric to set value or reset the pointer. If caching, the metric won't > > destruct when we want to set value only, and this is a necessary property for > > the implementation. That's why we need this caching. > > > > The singe sample metric is designed in a way that it inserts a sample to the > > histogram when it destructs, so destruction should be used only when > necessary. > > I see. This function seems to create a new metric every time it's called so I > think the name GetOrCreate... is a little confusing, right? Whoops, you are right.
Thanks, lgtm.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2890173002/#ps180001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2890173002/#ps200001 (title: "fix duplicate creations of the metric")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2890173002/#ps220001 (title: "DEPS adjust")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
maxlg@chromium.org changed reviewers: + clamy@chromium.org, jwd@chromium.org
Hi Jesse, Camille, here are two files that requires Owner's approval. Could you please take a look at these files? Thanks! Jesse, could you please take a look at histograms.xml Camille, could you please take a look at render_frame_impl.cc
Sorry, I stopped watching closely after the earlier questions about navigation start. I'm a content/ owner, so I can help there as well. A few questions about where you've ended up, since I'm not sure I understand it yet. https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:3663: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); Why do we need both? https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1650: void RendererSchedulerImpl::OnCommitProvisionalLoad() { nit: This should be after OnNavigationStarted (to match order in .h file). https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:156: virtual void OnCommitProvisionalLoad() = 0; This needs a comment, especially for how it differs from OnNavigationStarted. (I'm not clear why we need both, if they're called back-to-back.)
https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:3663: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); On 2017/06/02 22:38:36, Charlie Reis wrote: > Why do we need both? There are two OnNavigationStarted(). One is this; another is in RenderFrameImpl::OnNavigate(). By experiment, it shows that OnNavigate() is not triggered from the second navigation of the same renderer, but this line is triggered on every navigation start. That's why we need a different signal from OnNavigationStarted(). I guess OnCommitProvisionalLoadAndNavigationStarted() will be more precise here, based on the position of the code. Do you think so? https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1650: void RendererSchedulerImpl::OnCommitProvisionalLoad() { On 2017/06/02 22:38:36, Charlie Reis wrote: > nit: This should be after OnNavigationStarted (to match order in .h file). Done. https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:156: virtual void OnCommitProvisionalLoad() = 0; On 2017/06/02 22:38:37, Charlie Reis wrote: > This needs a comment, especially for how it differs from OnNavigationStarted. > (I'm not clear why we need both, if they're called back-to-back.) I am not sure about what "Commit Provisional Load" serve as a general purpose and what it indicates, so I will call this function OnCommitProvisionalLoadAndNavigationStarted() to be more precise, to indicate this is signaling provisional load has been committed and navigation has started. Is it OK?
Hope the comments below clarify what I meant-- thanks. https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:3663: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); On 2017/06/02 23:32:06, Liquan (Max) Gu wrote: > On 2017/06/02 22:38:36, Charlie Reis wrote: > > Why do we need both? > > There are two OnNavigationStarted(). One is this; another is in > RenderFrameImpl::OnNavigate(). By experiment, it shows that OnNavigate() is not > triggered from the second navigation of the same renderer, but this line is > triggered on every navigation start. > > That's why we need a different signal from OnNavigationStarted(). > > I guess OnCommitProvisionalLoadAndNavigationStarted() will be more precise here, > based on the position of the code. Do you think so? No, I think OnCommitProvisionalLoad was a better name. (See my other comment for more detail.) My confusion here was why we still had OnNavigationStarted at all, since I thought we were talking about removing it. But looking closer, it looks like you're hopefully planning to remove OnNavigationStarted in a followup CL as part of https://crbug.com/726341. If that's the case, I'm ok with it, but please include a TODO to remove OnNavigationStarted (citing bug 726341). https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:156: virtual void OnCommitProvisionalLoad() = 0; On 2017/06/02 23:32:06, Liquan (Max) Gu wrote: > On 2017/06/02 22:38:37, Charlie Reis wrote: > > This needs a comment, especially for how it differs from OnNavigationStarted. > > (I'm not clear why we need both, if they're called back-to-back.) > > I am not sure about what "Commit Provisional Load" serve as a general purpose > and what it indicates, so I will call this function > OnCommitProvisionalLoadAndNavigationStarted() to be more precise, to indicate > this is signaling provisional load has been committed and navigation has > started. Is it OK? No, sorry, I think that's more confusing. I think we're just meaning different things when saying "navigation started." In the navigation logic, we refer to "start" as the time when the navigation's network request is being made, which often unknown to the renderer process (even more so with PlzNavigate). "Commit" is the point at which the renderer process starts parsing and rendering the new page, and it seems to me that's the point in time that any renderer-side metrics should be interested in-- it's the point that activity stops happening in the old page and starts happening in the new page. In that sense, I would expect us to remove OnNavigationStarted and only have OnCommitProvisionalLoad. If I'm following things correctly, that's the plan in https://crbug.com/726341, and we can have comments and TODOs in this file to that effect.
On 2017/06/05 05:02:34, Charlie Reis wrote: > No, sorry, I think that's more confusing. > > I think we're just meaning different things when saying "navigation started." > In the navigation logic, we refer to "start" as the time when the navigation's > network request is being made, which often unknown to the renderer process (even > more so with PlzNavigate). "Commit" is the point at which the renderer process > starts parsing and rendering the new page, and it seems to me that's the point > in time that any renderer-side metrics should be interested in-- it's the point > that activity stops happening in the old page and starts happening in the new > page. > > In that sense, I would expect us to remove OnNavigationStarted and only have > OnCommitProvisionalLoad. If I'm following things correctly, that's the plan in > https://crbug.com/726341, and we can have comments and TODOs in this file to > that effect. Thanks for clearing this up. Triggering scheduling policy changes based on "commit" seems like the way to go.
LGTM https://codereview.chromium.org/2890173002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2890173002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3969: // Navigation start Add trailing period.
Patchset #13 (id:260001) has been deleted
Patchset #12 (id:240001) has been deleted
On 2017/06/05 15:54:46, tdresser wrote: > LGTM > > https://codereview.chromium.org/2890173002/diff/240001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc > (right): > > https://codereview.chromium.org/2890173002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:3969: > // Navigation start > Add trailing period. Done. Sorry I accidentally deleted the last patch.
https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2890173002/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:3663: render_thread_impl->GetRendererScheduler()->OnCommitProvisionalLoad(); On 2017/06/05 05:02:34, Charlie Reis wrote: > On 2017/06/02 23:32:06, Liquan (Max) Gu wrote: > > On 2017/06/02 22:38:36, Charlie Reis wrote: > > > Why do we need both? > > > > There are two OnNavigationStarted(). One is this; another is in > > RenderFrameImpl::OnNavigate(). By experiment, it shows that OnNavigate() is > not > > triggered from the second navigation of the same renderer, but this line is > > triggered on every navigation start. > > > > That's why we need a different signal from OnNavigationStarted(). > > > > I guess OnCommitProvisionalLoadAndNavigationStarted() will be more precise > here, > > based on the position of the code. Do you think so? > > No, I think OnCommitProvisionalLoad was a better name. (See my other comment > for more detail.) > > My confusion here was why we still had OnNavigationStarted at all, since I > thought we were talking about removing it. But looking closer, it looks like > you're hopefully planning to remove OnNavigationStarted in a followup CL as part > of https://crbug.com/726341. If that's the case, I'm ok with it, but please > include a TODO to remove OnNavigationStarted (citing bug 726341). Thanks for the detailed explanation. I have added a todo and will address onNavigationStart in a followup CL. https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h (right): https://codereview.chromium.org/2890173002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/scheduler/renderer/renderer_scheduler.h:156: virtual void OnCommitProvisionalLoad() = 0; On 2017/06/05 05:02:34, Charlie Reis wrote: > On 2017/06/02 23:32:06, Liquan (Max) Gu wrote: > > On 2017/06/02 22:38:37, Charlie Reis wrote: > > > This needs a comment, especially for how it differs from > OnNavigationStarted. > > > (I'm not clear why we need both, if they're called back-to-back.) > > > > I am not sure about what "Commit Provisional Load" serve as a general purpose > > and what it indicates, so I will call this function > > OnCommitProvisionalLoadAndNavigationStarted() to be more precise, to indicate > > this is signaling provisional load has been committed and navigation has > > started. Is it OK? > > No, sorry, I think that's more confusing. > > I think we're just meaning different things when saying "navigation started." > In the navigation logic, we refer to "start" as the time when the navigation's > network request is being made, which often unknown to the renderer process (even > more so with PlzNavigate). "Commit" is the point at which the renderer process > starts parsing and rendering the new page, and it seems to me that's the point > in time that any renderer-side metrics should be interested in-- it's the point > that activity stops happening in the old page and starts happening in the new > page. > > In that sense, I would expect us to remove OnNavigationStarted and only have > OnCommitProvisionalLoad. If I'm following things correctly, that's the plan in > https://crbug.com/726341, and we can have comments and TODOs in this file to > that effect. Thanks!
Thanks! LGTM.
The CQ bit was checked by maxlg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2890173002/#ps300001 (title: "fix test nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
metrics lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maxlg@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_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, creis@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2890173002/#ps320001 (title: "fix mock_renderer_scheduler")
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": 320001, "attempt_start_ts": 1496758896516960,
"parent_rev": "03891c2c9655606943ea905b11705bddd850166c", "commit_rev":
"206f88a3c6df64c70511446235ed6ff72a94ce9e"}
Message was sent while issue was closed.
Description was changed from ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will measure the max EQT from navigation start to navigation away as a first step. BUG=710449 ========== to ========== EQT: Record the max queueing time from navigation start to navigation away We want to record the maximum EQT in an N ms sliding window between TTI and when the user navigates away, yet TTI is not landed yet, so this CL will measure the max EQT from navigation start to navigation away as a first step. BUG=710449 Review-Url: https://codereview.chromium.org/2890173002 Cr-Commit-Position: refs/heads/master@{#477289} Committed: https://chromium.googlesource.com/chromium/src/+/206f88a3c6df64c70511446235ed... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001) as https://chromium.googlesource.com/chromium/src/+/206f88a3c6df64c70511446235ed... |
