Need detailed navigation metrics to diagnose site isolation performance issues |
||||||
Issue descriptionThe existing navigation to commit metrics have a few flaws 1. They are only logged for browser initiated navigation 2. They have a max value of 10s, which causes frequent overflows.
,
Mar 30 2018
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/702e026e628b7147396e263f780ebfe190f5845d commit 702e026e628b7147396e263f780ebfe190f5845d Author: Charlie Harrison <csharrison@chromium.org> Date: Fri Mar 30 21:17:39 2018 Add Navigation.StartToCommit histograms. The current Navigation.TimeToCommit histogram does not capture renderer initiated navigations, unlike the TimeToReadyToCommit and ReadyToCommitUntilCommit ones. Since the current TimeToCommit histogram is captured in a different location and has different meaning, it is marked as obsolete (to be removed in the near future) and a new one is added that is recorded in the same location where ReadyToCommitUntilCommit is logged, so they measure the same points in time. This CL also adds the following combination of variants {main frame, subframe, all frames} X {same process, cross process, all} X {back/fwd, reload, new nav, all} These metrics are critical for the launch of Site Isolation, and we will consider removing some variants if they prove to not be useful. Bug: 827592 Change-Id: Ie06c208ed27c6143ad6f19915f60f4478a755828 Reviewed-on: https://chromium-review.googlesource.com/980963 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#547274} [modify] https://crrev.com/702e026e628b7147396e263f780ebfe190f5845d/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/702e026e628b7147396e263f780ebfe190f5845d/content/browser/frame_host/navigation_handle_impl.h [modify] https://crrev.com/702e026e628b7147396e263f780ebfe190f5845d/tools/metrics/histograms/histograms.xml
,
Mar 30 2018
I'm going to close this out for now. If we need to add more breakout metrics for e.g. ReadyToCommit let's re-open and I'm happy to continue helping. If it's something more complicated, feel free to take ownership from me :)
,
Apr 2 2018
Took a look at the data this morning. X-process navigations seem to have large spikes at 0ms, for both main frames and subframes. Anyone have any idea why that could be? One idea is that these are navigations that are bypassing ReadyToCommitNavigation (e.g. ones that don't need the network), and so complete very quickly which can lead to spammy signals. There are about 1.5x counts for StartToCommits across all frames as there are ReadyToCommits.
,
Apr 2 2018
Comments 5-6: That's a really interesting finding! There's no similar spike at 0ms for the SameProcess metrics. I'm not sure I understand the theory you mention-- when would cross-process navigations bypass ReadyToCommitNavigation? Those are usually cross-site cases, but I'm probably just missing something. At first glace, I don't see what would make this happen exclusively for cross-process navigations, so I would expect it to be a bug. Might be worth opening a bug for (or re-opening this one if you want to keep the discussion here).
,
Apr 2 2018
Re-opening this one. This is probably a bug in the metric. NavigationHandleImpl keeps a member "is_same_process_" set initially to false. We set it to true in ReadyToCommit if the navigation is same-process. I think the bug is that there are navigations that do not go through ReadyToCommit and they are (all?) same-process. We should just initialize is_same_process to true in NavigationHandleImpl I think.
,
Apr 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d89cb33ab64848420e6eed720817d61e857108ae commit d89cb33ab64848420e6eed720817d61e857108ae Author: Charlie Harrison <csharrison@chromium.org> Date: Mon Apr 02 18:58:28 2018 Default is_same_process_ to true for navigation metrics Tests pending since we probably want to get this out in the very next canary to avoid messing up further metrics. Bug: 827592 Change-Id: I522e1880a78e1bb2e60eae5ff974e027a3b00676 Reviewed-on: https://chromium-review.googlesource.com/990153 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#547478} [modify] https://crrev.com/d89cb33ab64848420e6eed720817d61e857108ae/content/browser/frame_host/navigation_handle_impl.cc
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c04354be1160ddebbe61fa329b2c26fe924f1410 commit c04354be1160ddebbe61fa329b2c26fe924f1410 Author: Charlie Harrison <csharrison@chromium.org> Date: Fri Apr 06 05:42:35 2018 Add browser tests for new Navigation.StartToCommit metrics This CL also adds a TODO to fix subframe transition metrics, but does not actually fix them yet. Bug: 827592 Change-Id: I95835ad2911046b402a41d3e5dcedf7b7735911a Reviewed-on: https://chromium-review.googlesource.com/992392 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#548685} [modify] https://crrev.com/c04354be1160ddebbe61fa329b2c26fe924f1410/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/c04354be1160ddebbe61fa329b2c26fe924f1410/content/browser/frame_host/navigation_handle_impl_browsertest.cc
,
May 11 2018
Let me mark this as fixed for now. Nasko: feel free to reopen or open a new bug if we need to fix subframe load types. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by csharrison@chromium.org
, Mar 30 2018