Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
633083002:
Changes PlzNavitate histograms to try and simplify their multi-modal characteristic. (Closed)
DescriptionChanges PlzNavitate histograms to try and simplify their multi-modal characteristic.
Adds new navigation histograms split by having or not spawned a new renderer, moves UMA reports to commit time and discounts time spent on before-unload.
BUG=416877
Committed: https://crrev.com/a045c8cbbff767d639bf89101b59c0670c834eb0
Cr-Commit-Position: refs/heads/master@{#299457}
Patch Set 1 : Added new histograms, moved UMA reports to commit time. #Patch Set 2 : Discounting time spent on beforeunload, created navigation metrics data class and updated histograms.xml. #
Total comments: 24
Patch Set 3 : Addressed CR comments, renamed histograms using suffixes, other minor changes. #
Total comments: 12
Patch Set 4 : Addressed CR comments; simplified NavigatorImpl::NavigationMetricsData. #Patch Set 5 : Classify histograms from navigations issued from a previous session restore. #
Total comments: 14
Patch Set 6 : Addressed CR comments: mostly minor improvements and naming changes. #
Total comments: 10
Patch Set 7 : Addressed CR comments #
Messages
Total messages: 27 (6 generated)
Patchset #1 (id:1) has been deleted
carlosk@chromium.org changed reviewers: + clamy@chromium.org, isherman@chromium.org, nasko@chromium.org
isherman@: PTAL for histograms.xml nasko@, clamy@: PTAL As previously discussed as the initial histograms we gathered were multi-modal and somewhat confusing, they are being split up by having or not spawned a new renderer. They way I chose to do that was by adding a time stamp to when RenderProcessHost starts the new process and comparing that at commit time with the navigation start time: if the former is later than the latter, a new process was spawned. For the 4 added histograms I'm also discounting the time spent in beforeunload. The previous histograms are still there measuring the same times but were added the "Raw" prefix to their names.
https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13760: -<histogram name="Navigation.TimeToCommit" units="milliseconds"> Please mark the obsolete histograms as <obsolete> rather than deleting their descriptions. https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13764: + if it required the spawning of a new renderer process and discounting any nit: "new renderer process and discounting" -> "new renderer process. Discounts" (applies throughout) https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13765: + time spent with the beforeunload event. nit: "spent with" -> "spent in" (applies throughout) https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13769: +<histogram name="Navigation.TimeToCommitOldRenderer" units="milliseconds"> Optional: You might want to use a <histogram_suffixes> element to reduce the amount of repetition in these histogram descriptions.
On 2014/10/07 19:42:11, carlosk wrote: > isherman@: PTAL for histograms.xml > > nasko@, clamy@: PTAL > > As previously discussed as the initial histograms we gathered were multi-modal > and somewhat confusing, they are being split up by having or not spawned a new > renderer. They way I chose to do that was by adding a time stamp to when > RenderProcessHost starts the new process and comparing that at commit time with > the navigation start time: if the former is later than the latter, a new process > was spawned. > > For the 4 added histograms I'm also discounting the time spent in beforeunload. > The previous histograms are still there measuring the same times but were added > the "Raw" prefix to their names. Do you still need the Raw histograms? If you do, do you need them temporarily, or long term? Histograms are not completely free, so it's best to clean up obsolete ones if nobody is going to look at them anymore. On the other hand, if they're useful, then please leave them in -- the cost of any single histogram is fairly low.
https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:181: base::TimeTicks start_time, GURL url) : start_time_(start_time), url_(url) { Please use clang-format/git cl format or keep to the style guide. Parameters should be on their own lines. Same with initializers. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:192: return ! start_time_.is_null(); nit: no need for space between ! and start_time_. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; Is there a different way to judge this? I'd rather not add this method to RenderProcessHostImpl, if possible. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.h:91: class NavigationMetricsData { Don't nest classes like this for non-class specific behavior. Style guide says each class lives in a file of its own. https://codereview.chromium.org/633083002/diff/40001/content/public/browser/r... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/633083002/diff/40001/content/public/browser/r... content/public/browser/render_process_host.h:235: virtual const base::TimeTicks& GetInitTime() const = 0; Methods that are only used within content/ don't need to be put in the content/public interface class. Even if we leave this method in the implementation, please remove this.
Thanks for your comments. @ilya: I'll keep the old histograms for a while as they might help us for now. If they are found to be unnecessary I'll mark them as obsolete. I also reverted the change the previously existing Navigation.TimeToURLJobStart histogram was reported to UMA so that it works exactly as it used to before. That was to keep it consistent but it means that the two new ones summed up will differ in report counts to the previous. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:181: base::TimeTicks start_time, GURL url) : start_time_(start_time), url_(url) { On 2014/10/08 17:10:42, nasko wrote: > Please use clang-format/git cl format or keep to the style guide. > Parameters should be on their own lines. Same with initializers. Done! I had tried to run those tools before but only today I was finally able to. I was doing the updating of my local tree the wrong way and had some broken tools as a consequence. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:192: return ! start_time_.is_null(); On 2014/10/08 17:10:42, nasko wrote: > nit: no need for space between ! and start_time_. Done. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; On 2014/10/08 17:10:42, nasko wrote: > Is there a different way to judge this? I'd rather not add this method to > RenderProcessHostImpl, if possible. What I'm trying to separate here are navigations that did or did not caused the creation of a new renderer process as consequentially the former takes longer than the latter what should be the reason we're seeing a clearly bi-modal histogram. Comparing the navigation start time with the renderer process spawn time -- from the one that was finally assigned to that navigation -- was the simplest and surest method I could think for doing this. Can you think of another way of finding that out? I could also: * try and find if there's a way of getting that information from the SO's process data but this would possibly lead to more complicated, multi-platform code. * Remove that method from the RenderProcessHost interface but then have to do a cast to RenderProcessHostImpl when I get it. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.h:91: class NavigationMetricsData { On 2014/10/08 17:10:42, nasko wrote: > Don't nest classes like this for non-class specific behavior. Style guide says > each class lives in a file of its own. So I kinda followed what's described here: http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... It's still an inner/nested class but it's definition this time is properly hidden. Let me know if you think it's OK or if I should anyway move it out into a new file. The reason why I used an inner class to begin with was that this one is solely used by NavigatorImpl. But as it's used as a member I can't fully hide it inside the .CC file. https://codereview.chromium.org/633083002/diff/40001/content/public/browser/r... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/633083002/diff/40001/content/public/browser/r... content/public/browser/render_process_host.h:235: virtual const base::TimeTicks& GetInitTime() const = 0; On 2014/10/08 17:10:42, nasko wrote: > Methods that are only used within content/ don't need to be put in the > content/public interface class. Even if we leave this method in the > implementation, please remove this. Done. https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13760: -<histogram name="Navigation.TimeToCommit" units="milliseconds"> On 2014/10/07 20:23:45, Ilya Sherman wrote: > Please mark the obsolete histograms as <obsolete> rather than deleting their > descriptions. Acknowledged how this is supposed to be handled and I'll in fact revert them to the old names. For now I'd like to have them just in case they helps us better understand how these timings actually work but they might be made obsolete in the future. https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13764: + if it required the spawning of a new renderer process and discounting any On 2014/10/07 20:23:45, Ilya Sherman wrote: > nit: "new renderer process and discounting" -> "new renderer process. Discounts" > (applies throughout) Done. https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13765: + time spent with the beforeunload event. On 2014/10/07 20:23:45, Ilya Sherman wrote: > nit: "spent with" -> "spent in" (applies throughout) Done. https://codereview.chromium.org/633083002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:13769: +<histogram name="Navigation.TimeToCommitOldRenderer" units="milliseconds"> On 2014/10/07 20:23:45, Ilya Sherman wrote: > Optional: You might want to use a <histogram_suffixes> element to reduce the > amount of repetition in these histogram descriptions. This is nice and fits like a glove, especially now that I realized I'd better keep the old behavior and names (not add the "Raw" suffix) to keep the existing metrics as they are right now.
Thanks, a few comments as I was reading along. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; On 2014/10/08 17:10:42, nasko wrote: > Is there a different way to judge this? I'd rather not add this method to > RenderProcessHostImpl, if possible. I think you could easily distinguish between the cases of a cross-process navigation, or one where the renderer was not live, and same-process navigations where the renderer is live. Granted for cross-process we may not end up creating a new renderer, but there should be a fairly good chance that we do, right? https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.h:91: class NavigationMetricsData { On 2014/10/09 16:53:45, carlosk wrote: > On 2014/10/08 17:10:42, nasko wrote: > > Don't nest classes like this for non-class specific behavior. Style guide says > > each class lives in a file of its own. > > So I kinda followed what's described here: > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... > > It's still an inner/nested class but it's definition this time is properly > hidden. Let me know if you think it's OK or if I should anyway move it out into > a new file. > > The reason why I used an inner class to begin with was that this one is solely > used by NavigatorImpl. But as it's used as a member I can't fully hide it inside > the .CC file. Normally you are not supposed to change some class member to a scoped_ptr just so that it allows you to forward declare. However I do think that switching to a scoped_ptr is better there (we should only have the struct when we have an ongoing navigation). https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator.h:146: virtual void LogBeforeUnloadTime( Note: if we only discount the time it took to run the beforeunload event on the renderer side, we may still get a bi-modal distribution due to the waiting time for the BeforeUnload IPC to get process on the renderer side (due to javascript currently executing). We have to be okay with that. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:182: class NavigatorImpl::NavigationMetricsData { I think this should be a struct. If you keep it a class, make its members private with the right accessors. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:188: void Set(base::TimeTicks start_time, GURL url); I would remove those methods and use the scoped_ptr::reset method explicitly (see below). https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:397: navigation_data_->Set(navigation_start, entry.GetURL()); I would rather use an explicit reset on the scoped ptr with a new NavigationMetricData object. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:918: navigation_data_->Reset(); I would reset the scoped ptr explicitly (navigation_data.reset()).
Histograms LGTM, thanks.
Thanks. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; On 2014/10/09 19:02:32, clamy wrote: > On 2014/10/08 17:10:42, nasko wrote: > > Is there a different way to judge this? I'd rather not add this method to > > RenderProcessHostImpl, if possible. > > I think you could easily distinguish between the cases of a cross-process > navigation, or one where the renderer was not live, and same-process navigations > where the renderer is live. Granted for cross-process we may not end up creating > a new renderer, but there should be a fairly good chance that we do, right? As we're trying to eliminate the bi-modality this causes I am looking for a "for sure" method: no guessing, no mistakes. That's why comparing timestamps seemed like a sure and simple way to go. The difference I see here is that trying to figure that out before the navigation is impossible as it's exactly the same problem that leads us to *speculative* spawn a new renderer for PlzNavigate. On the other hand I didn't know what are the post-navigation evidences that I could track that would tell me for sure that the navigation that just happened spawned or didn't spawn a new renderer. Do they exist? https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.h (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.h:91: class NavigationMetricsData { On 2014/10/09 19:02:32, clamy wrote: > On 2014/10/09 16:53:45, carlosk wrote: > > On 2014/10/08 17:10:42, nasko wrote: > > > Don't nest classes like this for non-class specific behavior. Style guide > says > > > each class lives in a file of its own. > > > > So I kinda followed what's described here: > > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... > > > > It's still an inner/nested class but it's definition this time is properly > > hidden. Let me know if you think it's OK or if I should anyway move it out > into > > a new file. > > > > The reason why I used an inner class to begin with was that this one is solely > > used by NavigatorImpl. But as it's used as a member I can't fully hide it > inside > > the .CC file. > > Normally you are not supposed to change some class member to a scoped_ptr just > so that it allows you to forward declare. However I do think that switching to a > scoped_ptr is better there (we should only have the struct when we have an > ongoing navigation). Acknowledged. I'll also ask in chromium-dev asking about this case as suggested by nasko@. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator.h:146: virtual void LogBeforeUnloadTime( On 2014/10/09 19:02:32, clamy wrote: > Note: if we only discount the time it took to run the beforeunload event on the > renderer side, we may still get a bi-modal distribution due to the waiting time > for the BeforeUnload IPC to get process on the renderer side (due to javascript > currently executing). We have to be okay with that. So you mean that javascript CPU load could delay the processing of the FrameMsg_BeforeUnload IPC message on the renderer side? I though that javascript would only play a role inside RenderFrameImpl::OnBeforeUnload @ line 1027: bool proceed = frame_->dispatchBeforeUnloadEvent(); I understand that for PlzNavigate we'd probably want to remove the dependency on the current javascript load and I think that might be the way to go. But the risk there is that we'll be ignoring this user-facing delay which is not due to his own action delay (of pressing the "Leave page" button quickly) like the current measured time is. Maybe also log that wait time? <time for the full round trip call from the browser> - <currently measured before-unload dispatching time> I'm not sure... WDYT? https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:182: class NavigatorImpl::NavigationMetricsData { On 2014/10/09 19:02:32, clamy wrote: > I think this should be a struct. If you keep it a class, make its members > private with the right accessors. Done. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:188: void Set(base::TimeTicks start_time, GURL url); On 2014/10/09 19:02:32, clamy wrote: > I would remove those methods and use the scoped_ptr::reset method explicitly > (see below). Done! Much better indeed! https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:397: navigation_data_->Set(navigation_start, entry.GetURL()); On 2014/10/09 19:02:32, clamy wrote: > I would rather use an explicit reset on the scoped ptr with a new > NavigationMetricData object. Done. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:918: navigation_data_->Reset(); On 2014/10/09 19:02:32, clamy wrote: > I would reset the scoped ptr explicitly (navigation_data.reset()). Done.
Patchset #4 (id:260001) has been deleted
https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; On 2014/10/10 10:00:03, carlosk wrote: > On 2014/10/09 19:02:32, clamy wrote: > > On 2014/10/08 17:10:42, nasko wrote: > > > Is there a different way to judge this? I'd rather not add this method to > > > RenderProcessHostImpl, if possible. > > > > I think you could easily distinguish between the cases of a cross-process > > navigation, or one where the renderer was not live, and same-process > navigations > > where the renderer is live. Granted for cross-process we may not end up > creating > > a new renderer, but there should be a fairly good chance that we do, right? > > As we're trying to eliminate the bi-modality this causes I am looking for a "for > sure" method: no guessing, no mistakes. That's why comparing timestamps seemed > like a sure and simple way to go. > > The difference I see here is that trying to figure that out before the > navigation is impossible as it's exactly the same problem that leads us to > *speculative* spawn a new renderer for PlzNavigate. > > On the other hand I didn't know what are the post-navigation evidences that I > could track that would tell me for sure that the navigation that just happened > spawned or didn't spawn a new renderer. Do they exist? No that I know of. In any case, I am not sure we would be able to eliminate the bi-modality there. I also don't think that figuring that out before the navigation is impossible. This is different from PlzNavigate, in that the RenderFrameHost that we chose in NavigatorImpl::NavigateToEntry is the one that _will be used_ all the time. In PlzNavigate, we are basically betting that the redirects won't get us to a different site instance, and so that whatever we create before the navigation will still be valid when the navigation can commit, hence the "speculative" name. https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator.h:146: virtual void LogBeforeUnloadTime( On 2014/10/10 10:00:03, carlosk wrote: > On 2014/10/09 19:02:32, clamy wrote: > > Note: if we only discount the time it took to run the beforeunload event on > the > > renderer side, we may still get a bi-modal distribution due to the waiting > time > > for the BeforeUnload IPC to get process on the renderer side (due to > javascript > > currently executing). We have to be okay with that. > > So you mean that javascript CPU load could delay the processing of the > FrameMsg_BeforeUnload IPC message on the renderer side? I though that javascript > would only play a role inside RenderFrameImpl::OnBeforeUnload @ line 1027: > > bool proceed = frame_->dispatchBeforeUnloadEvent(); > > I understand that for PlzNavigate we'd probably want to remove the dependency on > the current javascript load and I think that might be the way to go. But the > risk there is that we'll be ignoring this user-facing delay which is not due to > his own action delay (of pressing the "Leave page" button quickly) like the > current measured time is. > > Maybe also log that wait time? > > <time for the full round trip call from the browser> - <currently measured > before-unload dispatching time> > > I'm not sure... WDYT? No the delay is between the moment we send a BeforeUnload IPC and the moment RenderFrameImpl::OnBeforeUnload is called. With the Unload event I measured up to 3s delay on really bad Android devices. That said, I think we should include it in those histograms. My comment is just pointing out that because of that, we will probably still have multi-modal distributions in our histograms.
Thanks! ilya@: could you PTAL at histograms.xml again? Sorry to change histograms.xml again but clamy@ showed me a way to find if a navigation was issued from a session restore (due to a crash or "continue where I left off" setting) and wanted to add that now as later on it would affect the other existing histograms. https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/40001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:890: site_instance->GetProcess()->GetInitTime() > navigation_data.start_time_; On 2014/10/10 19:59:12, clamy wrote: > On 2014/10/10 10:00:03, carlosk wrote: > > On 2014/10/09 19:02:32, clamy wrote: > > > On 2014/10/08 17:10:42, nasko wrote: > > > > Is there a different way to judge this? I'd rather not add this method to > > > > RenderProcessHostImpl, if possible. > > > > > > I think you could easily distinguish between the cases of a cross-process > > > navigation, or one where the renderer was not live, and same-process > > navigations > > > where the renderer is live. Granted for cross-process we may not end up > > creating > > > a new renderer, but there should be a fairly good chance that we do, right? > > > > As we're trying to eliminate the bi-modality this causes I am looking for a > "for > > sure" method: no guessing, no mistakes. That's why comparing timestamps seemed > > like a sure and simple way to go. > > > > The difference I see here is that trying to figure that out before the > > navigation is impossible as it's exactly the same problem that leads us to > > *speculative* spawn a new renderer for PlzNavigate. > > > > On the other hand I didn't know what are the post-navigation evidences that I > > could track that would tell me for sure that the navigation that just happened > > spawned or didn't spawn a new renderer. Do they exist? > > No that I know of. In any case, I am not sure we would be able to eliminate the > bi-modality there. I also don't think that figuring that out before the > navigation is impossible. This is different from PlzNavigate, in that the > RenderFrameHost that we chose in NavigatorImpl::NavigateToEntry is the one that > _will be used_ all the time. In PlzNavigate, we are basically betting that the > redirects won't get us to a different site instance, and so that whatever we > create before the navigation will still be valid when the navigation can commit, > hence the "speculative" name. Indeed we might not be able to eliminate the bi/multi modality. I'm just trying to split things up so that we can try and figure out why for instance 30% of TimeToCommit are taking above 1s in Windows machines. Yes, indeed it seems I should be able to figure out the did/didn't spawn a new renderer other ways -- apparently simpler with the current navigation implementation -- but it'd be a) more complicated nonetheless and b) it would have to be dealt differently for the current implementation and for PlzNavigate. I just think comparing time stamps is dead simple, always corrent and now invisible form other "packages" (as I did the change Nasko asked for a hid it from the public RPH interface). https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/633083002/diff/60001/content/browser/frame_ho... content/browser/frame_host/navigator.h:146: virtual void LogBeforeUnloadTime( On 2014/10/10 19:59:12, clamy wrote: > On 2014/10/10 10:00:03, carlosk wrote: > > On 2014/10/09 19:02:32, clamy wrote: > > > Note: if we only discount the time it took to run the beforeunload event on > > the > > > renderer side, we may still get a bi-modal distribution due to the waiting > > time > > > for the BeforeUnload IPC to get process on the renderer side (due to > > javascript > > > currently executing). We have to be okay with that. > > > > So you mean that javascript CPU load could delay the processing of the > > FrameMsg_BeforeUnload IPC message on the renderer side? I though that > javascript > > would only play a role inside RenderFrameImpl::OnBeforeUnload @ line 1027: > > > > bool proceed = frame_->dispatchBeforeUnloadEvent(); > > > > I understand that for PlzNavigate we'd probably want to remove the dependency > on > > the current javascript load and I think that might be the way to go. But the > > risk there is that we'll be ignoring this user-facing delay which is not due > to > > his own action delay (of pressing the "Leave page" button quickly) like the > > current measured time is. > > > > Maybe also log that wait time? > > > > <time for the full round trip call from the browser> - <currently measured > > before-unload dispatching time> > > > > I'm not sure... WDYT? > > No the delay is between the moment we send a BeforeUnload IPC and the moment > RenderFrameImpl::OnBeforeUnload is called. With the Unload event I measured up > to 3s delay on really bad Android devices. That said, I think we should include > it in those histograms. My comment is just pointing out that because of that, we > will probably still have multi-modal distributions in our histograms. That's precisely the delay I referred to. :) I'll add another histogram (in another CL) to track the difference between the browser side delay to get the whole of the on-before-unload even handled and the renderer side value we are using. If we have a significant number of samples with values above just a few 10ths of milliseconds than we'll have to see what to do about this situation.
Thanks! A few comments below. https://chromiumcodereview.appspot.com/633083002/diff/390001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/633083002/diff/390001/content/browser/... content/browser/frame_host/navigator_impl.cc:891: "Navigation.TimeToCommit_NewRenderer_Restored_BeforeUnloadDiscounted", Line should be under 80 characters. https://chromiumcodereview.appspot.com/633083002/diff/390001/content/browser/... content/browser/frame_host/navigator_impl.cc:894: "Navigation.TimeToURLJobStart_NewRenderer_Restored_BeforeUnloadDiscounted", Line should be under 80 characters. https://chromiumcodereview.appspot.com/633083002/diff/390001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/633083002/diff/390001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:54550: + <suffix name="OldRenderer_BeforeUnloadDiscounted" I would rather use the name ExistingRenderer instead of OldRenderer.
A few comments. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:800: // happened later than the navigation request nit: It will be useful to point out that this happens only with browser initiated navigations. Also end comment with a full stop. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:874: if (details.is_main_frame && navigation_data_ && nit: Code is a bit more readable if it is less indented. You can check the inverse of this condition and return early, this way the following code will be easier to follow. https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:646: init_time_ = base::TimeTicks::Now(); This is the time we call Init. This is not the time that the process is created, which is later in time. Does it make a difference in this case? https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:142: virtual const base::TimeTicks& GetInitTime() const; This method is no longer inherited, so why is it virtual? Do we plan to keep this method just for some data collection of for the long run?
Thanks for the comments. A few more histogram names changed due to me learning that restored tabs can actually reuse an existing renderer. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:800: // happened later than the navigation request On 2014/10/13 15:55:24, nasko wrote: > nit: It will be useful to point out that this happens only with browser > initiated navigations. Also end comment with a full stop. Done. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:874: if (details.is_main_frame && navigation_data_ && On 2014/10/13 15:55:24, nasko wrote: > nit: Code is a bit more readable if it is less indented. You can check the > inverse of this condition and return early, this way the following code will be > easier to follow. Done. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:891: "Navigation.TimeToCommit_NewRenderer_Restored_BeforeUnloadDiscounted", On 2014/10/13 15:28:33, clamy wrote: > Line should be under 80 characters. Done. https://codereview.chromium.org/633083002/diff/390001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:894: "Navigation.TimeToURLJobStart_NewRenderer_Restored_BeforeUnloadDiscounted", On 2014/10/13 15:28:33, clamy wrote: > Line should be under 80 characters. Done. https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:646: init_time_ = base::TimeTicks::Now(); On 2014/10/13 15:55:24, nasko wrote: > This is the time we call Init. This is not the time that the process is created, > which is later in time. Does it make a difference in this case? It does not make a difference. I updated its getter comment to better explain what it is. https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/633083002/diff/390001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:142: virtual const base::TimeTicks& GetInitTime() const; On 2014/10/13 15:55:24, nasko wrote: > This method is no longer inherited, so why is it virtual? > Do we plan to keep this method just for some data collection of for the long > run? Done, not virtual anymore. I forgot to update it after that change. I'm also flagging it as being a PlzNavigate thingie. https://codereview.chromium.org/633083002/diff/390001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/633083002/diff/390001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:54550: + <suffix name="OldRenderer_BeforeUnloadDiscounted" On 2014/10/13 15:28:33, clamy wrote: > I would rather use the name ExistingRenderer instead of OldRenderer. Done.
LGTM with a few nits. https://codereview.chromium.org/633083002/diff/480001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/480001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:889: "Navigation.TimeToCommit_SessionRestored_BeforeUnloadDiscounted", In session restore we shouldn't be running beforeunload handlers. Are we doing it? If not, we can drop the suffix on the string. https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:264: // new renderer process was spawned); further calls to Init won't change this nit: s/spawned/created/ https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // NOTE: Will disappear after PlzNavitate is implemented. nit: s/implemented/completed/
Patchset #6 (id:480001) has been deleted
Patchset #6 (id:570001) has been deleted
Thanks! Replying down here as I dumb-fully deleted the patchset where you made them... Sorry! On 2014/10/13 18:13:38, nasko wrote: > LGTM with a few nits. > > https://codereview.chromium.org/633083002/diff/480001/content/browser/frame_h... > File content/browser/frame_host/navigator_impl.cc (right): > > https://codereview.chromium.org/633083002/diff/480001/content/browser/frame_h... > content/browser/frame_host/navigator_impl.cc:889: > "Navigation.TimeToCommit_SessionRestored_BeforeUnloadDiscounted", > In session restore we shouldn't be running beforeunload handlers. Are we doing > it? If not, we can drop the suffix on the string. Yes, that is true. It doesn't make a difference in terms of the actual code but it does in terms of naming. And it was because of that tiny change that I deleted the previous patch-set before noticing your comments were there. > https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... > File content/browser/renderer_host/render_process_host_impl.h (right): > > https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... > content/browser/renderer_host/render_process_host_impl.h:264: // new renderer > process was spawned); further calls to Init won't change this > nit: s/spawned/created/ Done. > https://codereview.chromium.org/633083002/diff/480001/content/browser/rendere... > content/browser/renderer_host/render_process_host_impl.h:266: // NOTE: Will > disappear after PlzNavitate is implemented. > nit: s/implemented/completed/ Done.
Histograms still LGTM, thanks. https://codereview.chromium.org/633083002/diff/610001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/633083002/diff/610001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13881: + Time between the start of a browser started navigation request in and its Optional nit: Perhaps "browser-started" rather than "browser started"? (Ditto below)
Thanks! LGTM with a few nits. https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... content/browser/frame_host/navigator_impl.cc:895: } else { nit: return early in the if above so that the code below does not need to be indented. https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... content/browser/frame_host/navigator_impl.cc:907: } else { nit: again return early in the if to drop the indentation of the code below. https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... File content/browser/renderer_host/render_process_host_impl.h (right): https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... content/browser/renderer_host/render_process_host_impl.h:263: // Getter for the time the first call to Init completed successfully (after a nit: s/Getter for/Returns https://chromiumcodereview.appspot.com/633083002/diff/610001/content/browser/... content/browser/renderer_host/render_process_host_impl.h:266: // NOTE: Will disappear after PlzNavitate is completed. nit: s/NOTE/Note
Thanks for all comments. All set and ready for commit. https://codereview.chromium.org/633083002/diff/610001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/633083002/diff/610001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:895: } else { On 2014/10/14 07:09:25, clamy wrote: > nit: return early in the if above so that the code below does not need to be > indented. Done... Even though I have my issues with duplicating the pointer reset (or almost any code for that matter). https://codereview.chromium.org/633083002/diff/610001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:907: } else { On 2014/10/14 07:09:25, clamy wrote: > nit: again return early in the if to drop the indentation of the code below. With the early return from the previous line now this is just a simple if-else and the char* line break was not required anymore so I just left it as is. https://codereview.chromium.org/633083002/diff/610001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/633083002/diff/610001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:263: // Getter for the time the first call to Init completed successfully (after a On 2014/10/14 07:09:25, clamy wrote: > nit: s/Getter for/Returns Done. https://codereview.chromium.org/633083002/diff/610001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:266: // NOTE: Will disappear after PlzNavitate is completed. On 2014/10/14 07:09:25, clamy wrote: > nit: s/NOTE/Note Done. https://codereview.chromium.org/633083002/diff/610001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/633083002/diff/610001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13881: + Time between the start of a browser started navigation request in and its On 2014/10/13 22:05:55, Ilya Sherman wrote: > Optional nit: Perhaps "browser-started" rather than "browser started"? (Ditto > below) Done.
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/633083002/680001
Message was sent while issue was closed.
Committed patchset #7 (id:680001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a045c8cbbff767d639bf89101b59c0670c834eb0 Cr-Commit-Position: refs/heads/master@{#299457} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
