Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
577963002:
Add time-to-network histogram considering browser side navigation (Closed)
DescriptionAdd new histogram to assert the improvements we should get with browser side navigation (aka PlzNavigate).
Time measured:
* Time from navigation request to first network request
* Time from navigation request to navigation committed/finished
BUG=416877
Committed: https://crrev.com/bb156a0490b0c2cf1ad155ab46fc4afa3fbb77c0
Cr-Commit-Position: refs/heads/master@{#297410}
Patch Set 1 #Patch Set 2 : Now I can get RFHs... But maybe not the right ones. :( #
Total comments: 10
Patch Set 3 : CR comments, changed start parameters storage, fixed wrong RFH info, initial URL check, initial com… #Patch Set 4 : Fixed navigation start being persisted for PlzNavigate code path as well #Patch Set 5 : removed debug logs and simplified to only add the time-to-network histogram #Patch Set 6 : histogram.xml #
Total comments: 27
Patch Set 7 : Addressed CR comments, added new hisogram, test and fixes. #
Total comments: 59
Patch Set 8 : Addressed CR comments. #Patch Set 9 : Minor changes for addressing CR comments #
Total comments: 12
Patch Set 10 : Addressed CR comments, bundled NavigatorImpl histogram related members into a tuple. #Patch Set 11 : Fixing new build issues #
Messages
Total messages: 30 (4 generated)
clamy@chromium.org changed reviewers: + clamy@chromium.org
Thanks, a few comments below. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:348: LOG(WARNING) << "routing_id / render_frame_host = " << render_frame_host->routing_id(); You cannot use the id values for this render_frame_host because it is not necessarily the one that is going to navigate. See further comments below. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:366: RenderFrameHostImpl* dest_render_frame_host = manager->Navigate(entry); We pick here the RFH that is going to navigate. It will be different from render_frame_host if we are doing a cross process navigation. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:397: dest_render_frame_host->Navigate(navigate_params); And we have it navigate here... https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:676: base::TimeDelta time_to_network = base::TimeTicks::Now() - timestamp; This should be timestamp - navigation_start_time_. Otherwise you are timing the time it takes to post a task to the UI thread.
Hi Camille. Thanks for your comments and here are mine back to you. :) I think I found the point where to properly store the navigation start data in NavigatorImpl, added the initial histogram for time-to-commit and the initial URL check (currently only for the time-to-network). I Also found the RenderFrameHostImpl::OnBeforeUnloadACK (and 2 other before-unload related methods) not inherited from the interface it's implemented and neither documented. But as it receives both a renderer_before_unload_start_time and a renderer_before_unload_end_time it seems like an ideal place to store the TimeDelta for the current navigation for later use on discounting it. Is this the right thing to do? If yes it seems that the right place to do it is with the other UMA stuff already in there (which I think are ppi@'s) but as there are a few early returns of this function I might be wrong. If that's not the right place, where should I get this information from? https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:348: LOG(WARNING) << "routing_id / render_frame_host = " << render_frame_host->routing_id(); On 2014/09/17 21:12:39, clamy wrote: > You cannot use the id values for this render_frame_host because it is not > necessarily the one that is going to navigate. See further comments below. Acknowledged. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:366: RenderFrameHostImpl* dest_render_frame_host = manager->Navigate(entry); On 2014/09/17 21:12:39, clamy wrote: > We pick here the RFH that is going to navigate. It will be different from > render_frame_host if we are doing a cross process navigation. Acknowledged. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:397: dest_render_frame_host->Navigate(navigate_params); On 2014/09/17 21:12:39, clamy wrote: > And we have it navigate here... So you mean that it's only here -- right before the Navigate call -- that I should store the navigation start time? And I should only do it if is_transfer_to_same is true? And ignore if it's false? https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:552: As there's this "no navigation happened" comment here I'm guessing I should only register the navigation right after this point. But... (see next comment) https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:557: But it's said here nothing should be added to this method anymore... I think this histogram work should be an exception to this rule as there is no other "component" involved because it's NavigatorImpl itself who's handling this data. https://codereview.chromium.org/577963002/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:676: base::TimeDelta time_to_network = base::TimeTicks::Now() - timestamp; On 2014/09/17 21:12:40, clamy wrote: > This should be timestamp - navigation_start_time_. Otherwise you are timing the > time it takes to post a task to the UI thread. Yes, of course! Done!
carlosk@chromium.org changed reviewers: + davidben@chromium.org, nasko@chromium.org
Dear reviewers: PTAL. This adds the first histogram we need for measuring the improvements we should get with browser side navigation: time-to-network (delay from navigation request to the 1st network request). NOTE: I added the entry to histograms.xml using a "funny" name because I'm not yet sure of how to name groups and histograms themselves. Your comments are welcome on this matter. :) FYI: Following a suggestion from ppi@ I'm considering moving/refactoring the histogram-specific code into a new file (possibly implementing WebContentsObserver).
Thanks! Please find my comments below. https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/frame_host/navigator.h:133: virtual void LogResourceRequestTime(base::TimeTicks timestamp, GURL url) {}; Make it a const GURL& . https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/frame_host/navigator_impl.cc (left): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/frame_host/navigator_impl.cc:344: Did you remove the line on purpose? I think it is better to keep it there, since it underlines that the comment is only about the previous line (and not the following two). https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/frame_host/navigator_impl.cc:677: LOCAL_HISTOGRAM_TIMES("PlzNavigate.TimeToNetworkRequest", time_to_network); You want a UMA_HISTOGRAM_TIMES, otherwise we will not get values reported from the field. Additionally, I think it would be good to reset the navigation_start_time_ and navigation_start_url_ here. Also add a condition to only execute this part of the code if navigation_start_time_ and navigation_start_url_ are not null. You also need to subtract the time executing the beforeunload event from this value, as we discussed. https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/frame_host/navigator_impl.h (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/frame_host/navigator_impl.h:12: #include "content/common/content_export.h" You need to include url/gurl.h since you now have a class GURL member down there. https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/frame_host/navigator_impl.h:111: // The time the latest navigation request started for histogram building nit: How about: The time the latest navigation request started, used in Navigation.TimeToURLJobStart (or whatever we end up calling the histogram). https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.cc:346: content::Navigator* navigator = host->frame_tree_node()->navigator(); content:: is unnecessary here. https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.cc:911: int child_id = filter_->child_id(); nit: you could just write filter_->child_id() in the PostTask below. https://chromiumcodereview.appspot.com/577963002/diff/100001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:23924: +<histogram name="PlzNavigate.TimeToNetworkRequest" units="milliseconds"> We have a section of histograms called Navigation, so I suggest renaming it Navigation.TimeToURLJobStart. https://chromiumcodereview.appspot.com/577963002/diff/100001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:23927: + Time delay from a navigation request until the first network request is I think the following description is more accurate: Time between the start of a navigation request in the browser and the reception of a corresponding ResourceRequest in the network stack.
https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called from the IO thread when the first resource request for a given Nit: I would maybe say 'Called from the loader when' or just 'Called when'. I read 'IO thread' at first to mean that this gets run on the IO thread which made me nervous. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:675: if (navigation_start_url_ == url) { This is slightly racy, but there's not really much we can do about it (that this is awkward is something PlzNavigate is going to fix since we can tie everything to a NavigationRequest). Just making a note of it: we're correlating events out of flow without any IDs, so if we navigate twice in a row to the same URL (say first a GET then a POST), the GET request starting (and being canceled shortly) may get associated with the navigation_start_time_ of the POST. https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:340: GURL url) { Nit: const GURL& url https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:916: TimeTicks::Now(), A thought: net/ maintains a net::LoadTimingInfo which already has a request_start in it. It's not quite the same; that measures when //net starts processing the request so it wouldn't include, say, SafeBrowsing checks. net::LoadTiming is on the content::ResourceResponse. I wonder if it would be better to extract it from there. In https://codereview.chromium.org/519533002/, it'll be available on the ResourceResponse we get back when the navigation commits. https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_timi... (It would also be easy enough for NavigationURLLoader to send this variant of request_start through its Delegate, but this one means not introducing a new variant.)
Thanks for the previous comments. clamy, nasko, davidben and isherman (welcome!): PTAL. We have a new issue for this CL as referred in the updated description. I created a simple design doc and added the link there (comments are welcome): https://docs.google.com/a/chromium.org/document/d/1rvaepRDO9rhsN79hryz48Smal5... So I've broaden the spectrum of this CL to include time-to-navigation-finished again. It was finally an easy addition and better to add it now rather than later. Also added a simple test for checking that the histograms are being worked for both regular and PlzNavigate navigation. As noted it doesn't track the time-to-network though as if I understand correctly the IO thread is not really running during those tests. I noticed these histograms are also tracking navigation to "chrome-extension://..." URLS. Are these really supposed to be tracked? Camille mentioned there might be a problem with "navigation transfer requests" but I'm uncertain on what to do about those (or maybe I just forgot what she told me I should do). :) I also have the impression of having read somewhere that there was some script I should run when adding new histograms. Anyone knows about that? https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called from the IO thread when the first resource request for a given On 2014/09/19 21:52:45, David Benjamin wrote: > Nit: I would maybe say 'Called from the loader when' or just 'Called when'. I > read 'IO thread' at first to mean that this gets run on the IO thread which made > me nervous. Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:133: virtual void LogResourceRequestTime(base::TimeTicks timestamp, GURL url) {}; On 2014/09/19 14:56:03, clamy wrote: > Make it a const GURL& . Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (left): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:344: On 2014/09/19 14:56:03, clamy wrote: > Did you remove the line on purpose? I think it is better to keep it there, since > it underlines that the comment is only about the previous line (and not the > following two). Done. It was not on purpose: I did changes in this line previously and than not fully reverted them. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:675: if (navigation_start_url_ == url) { On 2014/09/19 21:52:45, David Benjamin wrote: > This is slightly racy, but there's not really much we can do about it (that this > is awkward is something PlzNavigate is going to fix since we can tie everything > to a NavigationRequest). Just making a note of it: we're correlating events out > of flow without any IDs, so if we navigate twice in a row to the same URL (say > first a GET then a POST), the GET request starting (and being canceled shortly) > may get associated with the navigation_start_time_ of the POST. Yes! I was from the start looking for a "request_id" that would be consistent throughout a navigation to more easily match this! But as this doesn't yet exist I'll have to play with the URL for now. I also added a TODO (to the navigatior.h method) stating this should be updated once we have something more consistent. Also if ppi@ suggestion of moving this histogram collection into a WebContentsObserver works, this tracking shouldn't be required at all. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:677: LOCAL_HISTOGRAM_TIMES("PlzNavigate.TimeToNetworkRequest", time_to_network); On 2014/09/19 14:56:03, clamy wrote: > You want a UMA_HISTOGRAM_TIMES, otherwise we will not get values reported from > the field. Done! My bad! > Additionally, I think it would be good to reset the navigation_start_time_ and > navigation_start_url_ here. Also add a condition to only execute this part of > the code if navigation_start_time_ and navigation_start_url_ are not null. Done! A reset here was indeed the right thing as it was the only histogram that required it. But I just added the time-to-having-navigated histogram so the reset moved there. As these values are not pointers I can't sent them to null. So I'm setting navigation_start_time_ = TimeTicks() (equivalent of "0") and using that as a flag. It's not required to also reset the URL so I'm not doing it; let me know if I should for some reason. > You also need to subtract the time executing the beforeunload event from this > value, as we discussed. Oh! I haven't previously understood that the beforeunload should also be discounted from this measurement! I thought it was only required for the time-to-commit! But as discussed yesterday I am for now dropping the need to subtract it. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:12: #include "content/common/content_export.h" On 2014/09/19 14:56:03, clamy wrote: > You need to include url/gurl.h since you now have a class GURL member down > there. Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:111: // The time the latest navigation request started for histogram building On 2014/09/19 14:56:03, clamy wrote: > nit: How about: The time the latest navigation request started, used in > Navigation.TimeToURLJobStart (or whatever we end up calling the histogram). Done but I mentioned the group name instead as there will be more than one histogram based on this value. https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:340: GURL url) { On 2014/09/19 21:52:45, David Benjamin wrote: > Nit: const GURL& url Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:346: content::Navigator* navigator = host->frame_tree_node()->navigator(); On 2014/09/19 14:56:03, clamy wrote: > content:: is unnecessary here. Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:911: int child_id = filter_->child_id(); On 2014/09/19 14:56:03, clamy wrote: > nit: you could just write filter_->child_id() in the PostTask below. Done. https://codereview.chromium.org/577963002/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:916: TimeTicks::Now(), On 2014/09/19 21:52:45, David Benjamin wrote: > A thought: net/ maintains a net::LoadTimingInfo which already has a > request_start in it. It's not quite the same; that measures when //net starts > processing the request so it wouldn't include, say, SafeBrowsing checks. > net::LoadTiming is on the content::ResourceResponse. I wonder if it would be > better to extract it from there. In https://codereview.chromium.org/519533002/, > it'll be available on the ResourceResponse we get back when the navigation > commits. > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_timi... > > (It would also be easy enough for NavigationURLLoader to send this variant of > request_start through its Delegate, but this one means not introducing a new > variant.) As discussed in yesterday's hangout I'll use this information instead of the more convoluted way it's done now. That means that for adding the time-to-network histogram I'll wait on the CL https://codereview.chromium.org/519533002/ to be submitted so that the request_time is made available at navigation commit. https://codereview.chromium.org/577963002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/577963002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23924: +<histogram name="PlzNavigate.TimeToNetworkRequest" units="milliseconds"> On 2014/09/19 14:56:03, clamy wrote: > We have a section of histograms called Navigation, so I suggest renaming it > Navigation.TimeToURLJobStart. Done. https://codereview.chromium.org/577963002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:23927: + Time delay from a navigation request until the first network request is On 2014/09/19 14:56:03, clamy wrote: > I think the following description is more accurate: > Time between the start of a navigation request in the browser and the reception > of a corresponding ResourceRequest in the network stack. Done.
isherman@chromium.org changed reviewers: + isherman@chromium.org
Are you familiar with the PLT.PT_* metrics? These acronyms are pretty opaque, but they stand for "PageLoadTime.PageTiming" (I think). I'm not super familiar with the details, but I suspect that either (a) there's already a metric there that you can use; or (b) you should add your metrics to that family of metrics. Please check with the owners of those metrics and get their thoughts.
Thanks! Please find a few comments below. @isherman: actually I think we are trying to phase out the PLT.PT* in favor of the PLT.NT* metrics. In any case, the PLT metrics are collected on the renderer side. Here we want to have browser side metrics that track a specific time delta in the navigation, in order to show the improvements brought by switching navigations to be browser-driven. There is also a bug for making equivalents of the PLT metrics but collected on the browser side in order to compare both of them (see crbug.com/411261). Since PLT.NT* metrics are based on the NavigationTiming API, and those are not, we did not want to name them as PLT.NT* in order not to confuse people. https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/100001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.cc:916: TimeTicks::Now(), On 2014/09/23 17:02:57, carlosk wrote: > On 2014/09/19 21:52:45, David Benjamin wrote: > > A thought: net/ maintains a net::LoadTimingInfo which already has a > > request_start in it. It's not quite the same; that measures when //net starts > > processing the request so it wouldn't include, say, SafeBrowsing checks. > > net::LoadTiming is on the content::ResourceResponse. I wonder if it would be > > better to extract it from there. In > https://codereview.chromium.org/519533002/, > > it'll be available on the ResourceResponse we get back when the navigation > > commits. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_timi... > > > > (It would also be easy enough for NavigationURLLoader to send this variant of > > request_start through its Delegate, but this one means not introducing a new > > variant.) > > As discussed in yesterday's hangout I'll use this information instead of the > more convoluted way it's done now. That means that for adding the > time-to-network histogram I'll wait on the CL > https://codereview.chromium.org/519533002/ to be submitted so that the > request_time is made available at navigation commit. As mentioned in the bug, this is problematic as https://codereview.chromium.org/519533002/ will only make it available for the PlzNavigate architecture. But this TimeToNetwork histogram is mostly interesting for the current architecture, where we should see a significant delay on Android. Therefore, we should have a solution that works in the current version of the code as well. https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... content/browser/frame_host/navigator_impl.cc:560: // that the observer methods are actually called. If you don't implement the histograms as an observer, I would rephrase this comment as TODO(carlosk): Move this out when PlzNavigate implementation properly calls the observer methods. Alternatively, having PlzNavigate call the observer method for AboutToNavigateRenderView should be fairly simple. The problem there will be the plumbing required to get the network start time to the implementation of WebContentsObserver used to record the histograms. https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); I would rename this Navigation.TimeToCommit. https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... File content/browser/frame_host/navigator_impl.h (right): https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... content/browser/frame_host/navigator_impl.h:116: // active when recording histogram data. s/the later matching with the active/later matching https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1868: TEST_F(RenderFrameHostManagerTest, You could test that two navigation commits result only in one value being recorded if there was no NavigateToEntry in between. Otherwise I am not sure this test is super useful. https://chromiumcodereview.appspot.com/577963002/diff/120001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/577963002/diff/120001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:13587: +<histogram name="Navigation.TimeToNavigationFinished" units="milliseconds"> As mentioned earlier I think TimeToCommit would be a better name IMO. https://chromiumcodereview.appspot.com/577963002/diff/120001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:13590: + Time between the start of a navigation request in the browser and it having it having finished navigating -> its commit.
On 2014/09/23 21:54:08, clamy wrote: > @isherman: actually I think we are trying to phase out the PLT.PT* in favor of > the PLT.NT* metrics. In any case, the PLT metrics are collected on the renderer > side. Here we want to have browser side metrics that track a specific time delta > in the navigation, in order to show the improvements brought by switching > navigations to be browser-driven. There is also a bug for making equivalents of > the PLT metrics but collected on the browser side in order to compare both of > them (see crbug.com/411261). Since PLT.NT* metrics are based on the > NavigationTiming API, and those are not, we did not want to name them as PLT.NT* > in order not to confuse people. Thanks for the explanation -- that's good to know! Would it perhaps still be useful to house these metrics under the "PLT" brand, though? It seems like it would be nice to have a single metrics group that's responsible for all things page load time. WDYT?
Some comments, most are nits. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called when the first resource request for a given navigation is executed nit: Is there a second/subsequent resource request? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:134: // sure/consistent once we have this something available (PlzNavigate should nit: Let's try to avoid using "we". https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:561: if (details.is_main_frame && Do we only care about top-level navigations? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:564: base::TimeDelta time_to_network = Why is this named "time_to_network"? I don't see much networking here. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:565: base::TimeTicks::Now() - navigation_start_time_; nit: wrong indent https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); On 2014/09/23 21:54:07, clamy wrote: > I would rename this Navigation.TimeToCommit. +1. I'd rename the TimeDelta variable also to match this. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:691: // TODO(carlosk): when Issue 376015 done (CL 519533002) use the Instead of using "issue XXXXXX", use http://crbug.com/XXXXXX, which allows us to link directly to the bug. Also, avoid putting CL numbers, since those are a lot more volatile. One can open the bug and look at the recorded CL to get to it. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:693: // time stamp for time-to-network and replace the more convoluted way it's nit: timestamp https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:115: // The URL of the navigation request to allow the later matching with the nit: leave an empty line above the comment https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:117: GURL navigation_start_url_; Why not use a pair of (url, frame_tree_node_id)? That will disambiguate much better than just the URL? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1866: // Note that as the IO thread is not really running the nit: Usually the format of notes is "Note: " https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Navigation.TimeToURLJobStart histogram cannot be tracked here. This comment reads strangely. Break needed between "running" and "the"? https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:335: // request to the respective NavigatorImpl (for an UMA histogram). nit: Navigator, you don't have any Impls in the code below. https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:339: int render_frame_host, nit: render_frame_id https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:346: Navigator* navigator = host->frame_tree_node()->navigator(); nit: no real need for a stack variable here.
On 2014/09/23 22:05:03, Ilya Sherman wrote: > On 2014/09/23 21:54:08, clamy wrote: > > @isherman: actually I think we are trying to phase out the PLT.PT* in favor of > > the PLT.NT* metrics. In any case, the PLT metrics are collected on the > renderer > > side. Here we want to have browser side metrics that track a specific time > delta > > in the navigation, in order to show the improvements brought by switching > > navigations to be browser-driven. There is also a bug for making equivalents > of > > the PLT metrics but collected on the browser side in order to compare both of > > them (see crbug.com/411261). Since PLT.NT* metrics are based on the > > NavigationTiming API, and those are not, we did not want to name them as > PLT.NT* > > in order not to confuse people. > > Thanks for the explanation -- that's good to know! Would it perhaps still be > useful to house these metrics under the "PLT" brand, though? It seems like it > would be nice to have a single metrics group that's responsible for all things > page load time. WDYT? Thanks for these explanations! @isherman and @clamy: As the idea is to break apart from the previous metrics it sounds reasonable to me to move these new browser-side navigation metrics to a new groups. But still grouping every and all similar metrics (no matter how and where they are measured) under the same PLT group doesn't sound unreasonable either, as long as we can find a new "sub-group" (PLT.XX*) that would tell them apart from the rest. I don't have a strong preference between these two. @isherman, you being the reviewer owner of the histograms.xml file I'd tend to go along with whatever you prefer.
Thanks everyone for reviewing. Most of your comments were DONE but a few were replied with comments or questions. Would you all PTAL again? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called when the first resource request for a given navigation is executed On 2014/09/24 00:48:06, nasko wrote: > nit: Is there a second/subsequent resource request? Can't there possibly be another one? I might be wrong but wouldn't redirects (and possibly other valid use cases) cause one single navigation request to make a 2nd network request? Or maybe a "resource request" not exactly what I'm assuming it is (just a request to the network)? I'm still too noob in this matter to know it for sure... https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:134: // sure/consistent once we have this something available (PlzNavigate should On 2014/09/24 00:48:06, nasko wrote: > nit: Let's try to avoid using "we". In fact, given that this "more precise identifier" will only be available for PlzNavigate (and not the current navigation) I completely removed this TODO. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:560: // that the observer methods are actually called. On 2014/09/23 21:54:07, clamy wrote: > If you don't implement the histograms as an observer, I would rephrase this > comment as > TODO(carlosk): Move this out when PlzNavigate implementation properly calls the > observer methods. > > Alternatively, having PlzNavigate call the observer method for > AboutToNavigateRenderView should be fairly simple. The problem there will be the > plumbing required to get the network start time to the implementation of > WebContentsObserver used to record the histograms. I'll rephrase that to what you suggested. Some notes on the change to a WCO implementation: * Yes, changing is simple and allows us not to have to worry about matching with the requests when what we need is offered by the WCO API. * And yes the problem is the network start time because we'd need another way of getting it (as we have now) and for that specific case we will need to do the matching with the request anyway * Finally the main reason I'm leaving this for later is because I think what I added isn't too much stuff (arguably, this CR will confirm or counter this point) and that currently the PlzNavigate work on eagerly spawning the rendered in parallel was considered more important https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:561: if (details.is_main_frame && On 2014/09/24 00:48:06, nasko wrote: > Do we only care about top-level navigations? Yes, from my talks with Camille. I still can't argue much in these cases but I assume top level navigation offer a more suitable use case to checkout the PlzNavigate improvements as for instance I guess non-top ones will never spawn a new renderer? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:564: base::TimeDelta time_to_network = On 2014/09/24 00:48:06, nasko wrote: > Why is this named "time_to_network"? I don't see much networking here. Done! Bad copy-paste skills! :) https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:565: base::TimeTicks::Now() - navigation_start_time_; On 2014/09/24 00:48:06, nasko wrote: > nit: wrong indent Done! I assumed I was missing an extra couple of spaces? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); On 2014/09/24 00:48:06, nasko wrote: > On 2014/09/23 21:54:07, clamy wrote: > > I would rename this Navigation.TimeToCommit. > > +1. I'd rename the TimeDelta variable also to match this. Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); On 2014/09/23 21:54:07, clamy wrote: > I would rename this Navigation.TimeToCommit. Done! I had it named that way because this happens in the DidNavigate method which comes *after* the CommitNavigation call. It seemed something "post commit". https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:691: // TODO(carlosk): when Issue 376015 done (CL 519533002) use the On 2014/09/24 00:48:06, nasko wrote: > Instead of using "issue XXXXXX", use http://crbug.com/XXXXXX, which allows us to > link directly to the bug. Also, avoid putting CL numbers, since those are a lot > more volatile. One can open the bug and look at the recorded CL to get to it. Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:693: // time stamp for time-to-network and replace the more convoluted way it's On 2014/09/24 00:48:06, nasko wrote: > nit: timestamp Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:115: // The URL of the navigation request to allow the later matching with the On 2014/09/24 00:48:07, nasko wrote: > nit: leave an empty line above the comment Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:116: // active when recording histogram data. On 2014/09/23 21:54:07, clamy wrote: > s/the later matching with the active/later matching Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:117: GURL navigation_start_url_; On 2014/09/24 00:48:07, nasko wrote: > Why not use a pair of (url, frame_tree_node_id)? That will disambiguate much > better than just the URL? I guess not because: * I don't think it'll help in the case of navigating twice to the same URL as I'm assuming the frame_tree_node_id won't change between them. * And as we only care about main frame navigation, if one navigates to the same URL in two different main frames they will have different NavigatorImpl and RenderFrameHostManager instances anyway, right? So the frame_tree_node_id wouldn't help either. Am I missing something? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1866: // Note that as the IO thread is not really running the On 2014/09/24 00:48:07, nasko wrote: > nit: Usually the format of notes is "Note: " Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Navigation.TimeToURLJobStart histogram cannot be tracked here. On 2014/09/24 00:48:07, nasko wrote: > This comment reads strangely. Break needed between "running" and "the"? I rephrased it to hopefully make it clearer. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1868: TEST_F(RenderFrameHostManagerTest, On 2014/09/23 21:54:07, clamy wrote: > You could test that two navigation commits result only in one value being > recorded if there was no NavigateToEntry in between. Otherwise I am not sure > this test is super useful. Done, added that case to the test. IMO this one is also important for it proves the metric is tracked both when PlzNavigate is enabled or disabled as the code-paths are slightly different. https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:335: // request to the respective NavigatorImpl (for an UMA histogram). On 2014/09/24 00:48:07, nasko wrote: > nit: Navigator, you don't have any Impls in the code below. Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:339: int render_frame_host, On 2014/09/24 00:48:07, nasko wrote: > nit: render_frame_id Done. https://codereview.chromium.org/577963002/diff/120001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:346: Navigator* navigator = host->frame_tree_node()->navigator(); On 2014/09/24 00:48:07, nasko wrote: > nit: no real need for a stack variable here. Done. https://codereview.chromium.org/577963002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/577963002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13587: +<histogram name="Navigation.TimeToNavigationFinished" units="milliseconds"> On 2014/09/23 21:54:07, clamy wrote: > As mentioned earlier I think TimeToCommit would be a better name IMO. Done. https://codereview.chromium.org/577963002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13590: + Time between the start of a navigation request in the browser and it having On 2014/09/23 21:54:07, clamy wrote: > it having finished navigating -> its commit. Done.
Looks nicer! Few more comments. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called when the first resource request for a given navigation is executed On 2014/09/24 18:35:49, carlosk wrote: > On 2014/09/24 00:48:06, nasko wrote: > > nit: Is there a second/subsequent resource request? > > Can't there possibly be another one? I might be wrong but wouldn't redirects > (and possibly other valid use cases) cause one single navigation request to make > a 2nd network request? Or maybe a "resource request" not exactly what I'm > assuming it is (just a request to the network)? > > I'm still too noob in this matter to know it for sure... That is a good point. I was thinking of "navigation" request, which there will be one, but you consider redirects as separate resource requests. Makes sense. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:565: base::TimeTicks::Now() - navigation_start_time_; On 2014/09/24 18:35:49, carlosk wrote: > On 2014/09/24 00:48:06, nasko wrote: > > nit: wrong indent > > Done! I assumed I was missing an extra couple of spaces? Yes. In general, clang-format is your friend for almost all formatting needs : ). https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); On 2014/09/24 18:35:50, carlosk wrote: > On 2014/09/23 21:54:07, clamy wrote: > > I would rename this Navigation.TimeToCommit. > > Done! > > I had it named that way because this happens in the DidNavigate method which > comes *after* the CommitNavigation call. It seemed something "post commit". I think you have a bit of a misconception of start, commit, finish events. It took me forever to figure out at the beginning, so I'd be happy to hop on chat and help clarify those. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:691: // TODO(carlosk): when Issue 376015 done (CL 519533002) use the On 2014/09/24 18:35:49, carlosk wrote: > On 2014/09/24 00:48:06, nasko wrote: > > Instead of using "issue XXXXXX", use http://crbug.com/XXXXXX, which allows us > to > > link directly to the bug. Also, avoid putting CL numbers, since those are a > lot > > more volatile. One can open the bug and look at the recorded CL to get to it. > > Done. And by "done" you mean "I've removed it, so it is no longer there"? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:117: GURL navigation_start_url_; On 2014/09/24 18:35:50, carlosk wrote: > On 2014/09/24 00:48:07, nasko wrote: > > Why not use a pair of (url, frame_tree_node_id)? That will disambiguate much > > better than just the URL? > > I guess not because: > > * I don't think it'll help in the case of navigating twice to the same URL as > I'm assuming the frame_tree_node_id won't change between them. > > * And as we only care about main frame navigation, if one navigates to the same > URL in two different main frames they will have different NavigatorImpl and > RenderFrameHostManager instances anyway, right? So the frame_tree_node_id > wouldn't help either. > > Am I missing something? I wrote this comment before asking about main frames. Since we only care with this about main frame timing, we will definitely have unique Navigators between tabs. What happens though if we start two navigations to the same URL quickly one after another? I think it is fine, right? https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Navigation.TimeToURLJobStart histogram cannot be tracked here. On 2014/09/24 18:35:50, carlosk wrote: > On 2014/09/24 00:48:07, nasko wrote: > > This comment reads strangely. Break needed between "running" and "the"? > > I rephrased it to hopefully make it clearer. Much better. Since it is a sentence, I'd start with capitalized The. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1868: TEST_F(RenderFrameHostManagerTest, On 2014/09/24 18:35:50, carlosk wrote: > On 2014/09/23 21:54:07, clamy wrote: > > You could test that two navigation commits result only in one value being > > recorded if there was no NavigateToEntry in between. Otherwise I am not sure > > this test is super useful. > > Done, added that case to the test. IMO this one is also important for it proves > the metric is tracked both when PlzNavigate is enabled or disabled as the > code-paths are slightly different. I don't quite follow. If PlzNavigate is disabled, how is the new case below testing that?
Thanks and here we go again. clamy, nasko, davidben, isherman: could you all let me know WDYT? My feeling is that this is GtG (or almost). @isherman: I would like to have your final word on the histogram naming choice given the discussions we had so far. I kept it as is for now but will adapt depending on what you have to say about it. Regards, Carlos. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator.h:131: // Called when the first resource request for a given navigation is executed On 2014/09/24 23:11:20, nasko wrote: > On 2014/09/24 18:35:49, carlosk wrote: > > On 2014/09/24 00:48:06, nasko wrote: > > > nit: Is there a second/subsequent resource request? > > > > Can't there possibly be another one? I might be wrong but wouldn't redirects > > (and possibly other valid use cases) cause one single navigation request to > make > > a 2nd network request? Or maybe a "resource request" not exactly what I'm > > assuming it is (just a request to the network)? > > > > I'm still too noob in this matter to know it for sure... > > That is a good point. I was thinking of "navigation" request, which there will > be one, but you consider redirects as separate resource requests. Makes sense. Cool. And in fact that makes me think I might need a way to check I'm not handling a 2nd call to this. I talked to clamy@ about it and will check for transfer_ids (because apparently redirects won't in fact cause this effect). See changes in ResourceDispatcherHostImpl::OnRequestResource for this. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:565: base::TimeTicks::Now() - navigation_start_time_; On 2014/09/24 23:11:20, nasko wrote: > On 2014/09/24 18:35:49, carlosk wrote: > > On 2014/09/24 00:48:06, nasko wrote: > > > nit: wrong indent > > > > Done! I assumed I was missing an extra couple of spaces? > > Yes. In general, clang-format is your friend for almost all formatting needs : > ). Didn't know about it and I'm having issues trying to run it locally. I'll figure it out later... https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:566: UMA_HISTOGRAM_TIMES("Navigation.TimeToNavigationFinished", time_to_network); On 2014/09/24 23:11:21, nasko wrote: > On 2014/09/24 18:35:50, carlosk wrote: > > On 2014/09/23 21:54:07, clamy wrote: > > > I would rename this Navigation.TimeToCommit. > > > > Done! > > > > I had it named that way because this happens in the DidNavigate method which > > comes *after* the CommitNavigation call. It seemed something "post commit". > > I think you have a bit of a misconception of start, commit, finish events. It > took me forever to figure out at the beginning, so I'd be happy to hop on chat > and help clarify those. Acknowledged. I'll take your teaching offer! :) https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:691: // TODO(carlosk): when Issue 376015 done (CL 519533002) use the On 2014/09/24 23:11:20, nasko wrote: > On 2014/09/24 18:35:49, carlosk wrote: > > On 2014/09/24 00:48:06, nasko wrote: > > > Instead of using "issue XXXXXX", use http://crbug.com/XXXXXX, which allows > us > > to > > > link directly to the bug. Also, avoid putting CL numbers, since those are a > > lot > > > more volatile. One can open the bug and look at the recorded CL to get to > it. > > > > Done. > > And by "done" you mean "I've removed it, so it is no longer there"? Yes I removed the full comment as it was applicable only for PlzNavigate (see in Patch Set 8). https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.h:117: GURL navigation_start_url_; On 2014/09/24 23:11:21, nasko wrote: > On 2014/09/24 18:35:50, carlosk wrote: > > On 2014/09/24 00:48:07, nasko wrote: > > > Why not use a pair of (url, frame_tree_node_id)? That will disambiguate much > > > better than just the URL? > > > > I guess not because: > > > > * I don't think it'll help in the case of navigating twice to the same URL as > > I'm assuming the frame_tree_node_id won't change between them. > > > > * And as we only care about main frame navigation, if one navigates to the > same > > URL in two different main frames they will have different NavigatorImpl and > > RenderFrameHostManager instances anyway, right? So the frame_tree_node_id > > wouldn't help either. > > > > Am I missing something? > > I wrote this comment before asking about main frames. Since we only care with > this about main frame timing, we will definitely have unique Navigators between > tabs. > What happens though if we start two navigations to the same URL quickly one > after another? I think it is fine, right? This is exactly the point that davidben@ brought up and the reason we're not completely happy with using the URL as the identifier. And yes that situation can happen but we're hoping it to be rare enough to not change significantly the results. From my understanding, for PlzNavigate we will have a consistent "request id" available throughout the navigation at some point in the future. But for the current navigation implementation this is the best we can have (without adding too much extra cost to this task). https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1867: // Navigation.TimeToURLJobStart histogram cannot be tracked here. On 2014/09/24 23:11:21, nasko wrote: > On 2014/09/24 18:35:50, carlosk wrote: > > On 2014/09/24 00:48:07, nasko wrote: > > > This comment reads strangely. Break needed between "running" and "the"? > > > > I rephrased it to hopefully make it clearer. > > Much better. Since it is a sentence, I'd start with capitalized The. Done: beginning with a capitalized "Tests". https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1868: TEST_F(RenderFrameHostManagerTest, On 2014/09/24 23:11:21, nasko wrote: > On 2014/09/24 18:35:50, carlosk wrote: > > On 2014/09/23 21:54:07, clamy wrote: > > > You could test that two navigation commits result only in one value being > > > recorded if there was no NavigateToEntry in between. Otherwise I am not sure > > > this test is super useful. > > > > Done, added that case to the test. IMO this one is also important for it > proves > > the metric is tracked both when PlzNavigate is enabled or disabled as the > > code-paths are slightly different. > > I don't quite follow. If PlzNavigate is disabled, how is the new case below > testing that? To better clarify what I'm testing with Patch Set 8: * I fully execute a current/regular navigation and test that it was registered * I the enable PlzNavigate and perform another full navigation and test it was registered again * I then simulate an in-tab rendered initiated navigation and confirm it's NOT registered (only with PlzNavigate enabled but I don't think this changes much in the other case). I also updated the test description to reflect this. You think the explanation there is not clear?
Regarding histogram naming, I think the owners of the existing PLT.* metrics are the best people to check with. My main concern was whether these metrics were redundant or not -- it sounds like there's agreement that they're not. My secondary concern is whether these are in the same spirit as the other PLT metrics. If they are, then it probably makes sense to group them together with the other PLT metrics, so that people looking for such metrics need only look in one place.
LGTM with a couple of nits. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:561: if (details.is_main_frame && On 2014/09/24 18:35:50, carlosk wrote: > On 2014/09/24 00:48:06, nasko wrote: > > Do we only care about top-level navigations? > > Yes, from my talks with Camille. I still can't argue much in these cases but I > assume top level navigation offer a more suitable use case to checkout the > PlzNavigate improvements as for instance I guess non-top ones will never spawn a > new renderer? Not yet, but in the not so long distant future they will : ). https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:910: // When logging time-to-network only cares about main frame and non-transfer nit: s/cares/care/ Why do we not care about transfers?
Thanks! LGTM with a few nits. https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/120001/content/browser/... content/browser/frame_host/navigator_impl.cc:561: if (details.is_main_frame && On 2014/09/26 00:10:35, nasko wrote: > On 2014/09/24 18:35:50, carlosk wrote: > > On 2014/09/24 00:48:06, nasko wrote: > > > Do we only care about top-level navigations? > > > > Yes, from my talks with Camille. I still can't argue much in these cases but I > > assume top level navigation offer a more suitable use case to checkout the > > PlzNavigate improvements as for instance I guess non-top ones will never spawn > a > > new renderer? > > Not yet, but in the not so long distant future they will : ). Though not on Android right? Since the delay we observe is mainly on Android, I think taking into account only the top level navigations may not be such an issue. https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... content/browser/frame_host/navigator.h:130: // PlzNavigate nit: This not PlzNavigate specific, so I think you should remove this line. https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... content/browser/frame_host/navigator.h:132: // so that we can time it. nit: Avoid using we in a comment. https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... content/browser/frame_host/render_frame_host_manager_unittest.cc:1883: // Performs an in-tab renderer initiated navigation I think it would make more sense to test that renderer-initiated navigations are not recorded in the non PlzNavigate case. With PlzNavigate they should eventually go to the browser and be recorded. https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://chromiumcodereview.appspot.com/577963002/diff/160001/content/browser/... content/browser/loader/resource_dispatcher_host_impl.cc:910: // When logging time-to-network only cares about main frame and non-transfer On 2014/09/26 00:10:35, nasko wrote: > nit: s/cares/care/ > Why do we not care about transfers? I think we only want the first request for transfers, not the second one that is made by the second renderer. I thought that checking we did not have a transferred_request_request_id would eliminate that case.
lgtm https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:913: request_data.transferred_request_request_id == -1) { Aside: There's a slight weirdness that this'll actually hit for the future blob URL fetch too, but the URL won't match, so it should be fine. We'll be able to correlate them better in the PlzNavigate version, but having one codepath work for both versions right now sounds like a good plan.
Thanks everyone! With the latest PS I addressed your previous comments and did a minor modification to NavigationImpl: as both member I added to track the start time and the URL are tied to each other I bundled them into a tuple to make that explicit. I'm now waiting on a reply from the owner of most PLT.NT* histograms regarding the naming discussion. https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/577963002/diff/120001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:561: if (details.is_main_frame && On 2014/09/26 14:48:32, clamy wrote: > On 2014/09/26 00:10:35, nasko wrote: > > On 2014/09/24 18:35:50, carlosk wrote: > > > On 2014/09/24 00:48:06, nasko wrote: > > > > Do we only care about top-level navigations? > > > > > > Yes, from my talks with Camille. I still can't argue much in these cases but > I > > > assume top level navigation offer a more suitable use case to checkout the > > > PlzNavigate improvements as for instance I guess non-top ones will never > spawn > > a > > > new renderer? > > > > Not yet, but in the not so long distant future they will : ). > > Though not on Android right? Since the delay we observe is mainly on Android, I > think taking into account only the top level navigations may not be such an > issue. Acknowledged. https://codereview.chromium.org/577963002/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator.h:130: // PlzNavigate On 2014/09/26 14:48:32, clamy wrote: > nit: This not PlzNavigate specific, so I think you should remove this line. Done. https://codereview.chromium.org/577963002/diff/160001/content/browser/frame_h... content/browser/frame_host/navigator.h:132: // so that we can time it. On 2014/09/26 14:48:32, clamy wrote: > nit: Avoid using we in a comment. Done. https://codereview.chromium.org/577963002/diff/160001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager_unittest.cc:1883: // Performs an in-tab renderer initiated navigation On 2014/09/26 14:48:32, clamy wrote: > I think it would make more sense to test that renderer-initiated navigations are > not recorded in the non PlzNavigate case. With PlzNavigate they should > eventually go to the browser and be recorded. Done! And updated the test comment to explain what's done. Should I add a TODO to update the test when PlzNavigate actually allows for that to be tracked? https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:910: // When logging time-to-network only cares about main frame and non-transfer On 2014/09/26 14:48:32, clamy wrote: > On 2014/09/26 00:10:35, nasko wrote: > > nit: s/cares/care/ > > Why do we not care about transfers? > > I think we only want the first request for transfers, not the second one that is > made by the second renderer. I thought that checking we did not have a > transferred_request_request_id would eliminate that case. Done. From my understanding of what we're tracking: because we want to measure only the time it took from a navigation request being started until only the 1st network request it caused, which is the delay we will be able to optimize with PlzNavigate. Measuring later ones due to a possible transfer doesn't seem useful in this context (we're not optimizing that). Does this sounds reasonable?
https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:913: request_data.transferred_request_request_id == -1) { On 2014/09/26 15:42:45, David Benjamin wrote: > Aside: There's a slight weirdness that this'll actually hit for the future blob > URL fetch too, but the URL won't match, so it should be fine. We'll be able to > correlate them better in the PlzNavigate version, but having one codepath work > for both versions right now sounds like a good plan. Acknowledged.
FWIW, I don't find the tuple based code easier to read, in fact, it looks a bit more strange. https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/577963002/diff/160001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:910: // When logging time-to-network only cares about main frame and non-transfer On 2014/09/26 15:50:25, carlosk wrote: > On 2014/09/26 14:48:32, clamy wrote: > > On 2014/09/26 00:10:35, nasko wrote: > > > nit: s/cares/care/ > > > Why do we not care about transfers? > > > > I think we only want the first request for transfers, not the second one that > is > > made by the second renderer. I thought that checking we did not have a > > transferred_request_request_id would eliminate that case. > > Done. > > From my understanding of what we're tracking: because we want to measure only > the time it took from a navigation request being started until only the 1st > network request it caused, which is the delay we will be able to optimize with > PlzNavigate. Measuring later ones due to a possible transfer doesn't seem useful > in this context (we're not optimizing that). Does this sounds reasonable? Acknowledged.
On 2014/09/26 17:34:19, nasko wrote: > FWIW, I don't find the tuple based code easier to read, in fact, it looks a bit > more strange. In my personal opinion it makes it clear that those are tied together, just like a poor/lazy man's struct. But you are the owner: if you think the other option was better I can revert it, no problem.
Per offline discussion, we'll go with the current Navigation.* names for the histograms. LGTM, and thanks for your patience.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/577963002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as 08f47d535854192c5c48270a3da99a0e714ca55c
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bb156a0490b0c2cf1ad155ab46fc4afa3fbb77c0 Cr-Commit-Position: refs/heads/master@{#297410} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
