This is my idea:
https://chromium-review.googlesource.com/c/chromium/src/+/815354

Nate, Tao: is this feasible? That is, is it OK to avoid cancelling the navigation right away if it should be ignored? The patch above just waits until the next NavigationThrottle callback.

Tomorrow we should be able to get data on main frame / subframe requests to see the potential wins, but Navigation.Intercept.WillStart shows pretty significant results atm.

Sorry, didn't get a chance to look at this yet. I'll try to take a look later today.

Sorry for the delay, I was stuck with bug triage last week.

How do the wins look for Android WebView? Our dashboard [1] doesn't look quite as bad as the chrome dashboard [2].

[1] https://uma.googleplex.com/p/android_webview/histograms?endDate=20171217&dayCount=1&histograms=Navigation.Intercept.WillStart&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial
[2] https://uma.googleplex.com/p/chrome/histograms/?endDate=20171217&dayCount=1&histograms=Navigation.Intercept.WillStart&fixupData=true&showMax=true&filters=isofficial%2Ceq%2CTrue&implicitFilters=isofficial

> Nate, Tao: is this feasible? That is, is it OK to avoid cancelling the navigation right away if it should be ignored? The patch above just waits until the next NavigationThrottle callback.

I'll have to take a closer look at the CL.

Yeah the webview data looks a lot better, though the counts are extremely low. If the codepath still goes through ExternalNavigationHandler.java I would be surprised if it isn't just as bad though.

That being said, I'm very unfamiliar with performance characteristics of webview.

There's a chance if we could get the java side code running on a separate thread we could realize some wins with a cleaner architecture (not allowing the request to be made at all) once we remove the non-PlzNavigate codepath. Not sure how feasible it is.

This was actually much worse before, see https://bugs.chromium.org/p/chromium/issues/detail?id=671609 (was fixed in https://bugs.chromium.org/p/chromium/issues/detail?id=732260)
:-)

From the metrics, the distribution is bimodal. I suspect that the slow path is the startup one, when the system is quite loaded, and either we are blocked on page faults somewhere or ActivityManager takes a lot of time to respond to the request. If so, the other tasks may be just as slow, but that's certainly worth a try.

Looking at the patch, perhaps the asynchronous check should be restricted to http and https schemes?

Thanks for the additional context! Yeah restricting on http/s schemes makes a lot of sense. I also suspect the slow path is startup, but it's interesting to me that the total weight in each modality is so similar.

Re other tasks being just as slow: I suspect this as well, but we're moving more and more into a world where the total work between nav start and the actual navigation is getting pretty small, so I think trying to parallelize the expensive stuff with the net request is tenable.

In all cases, shouldn't we have started to preconnect to the initial URL before this call?
If so, the likely outcome from the patch would be no improvement to time to commit in most cases.

Not to say that this is not the way to go. Generally speaking anything we can remove from the critical path would help at some point.

Not necessarily, with PlzNavigate, preconnects happen so late on the IO thread now that they actually started to delay the actual navigation request (cpu contention), so we moved preconnect to be dispatched after the nav (except for omnibox cases I believe).

I believe (but am not certain), this is how the new predictor works as well.

That preconnects peg the CPU for a while is another thing we could probably investigate though.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/003ae0aebb04d05bf187b8dbfcfdb165f78952a3

commit 003ae0aebb04d05bf187b8dbfcfdb165f78952a3
Author: Charles Harrison <csharrison@chromium.org>
Date: Thu Jan 04 21:15:05 2018

Convert navigation interception unit tests to use nav simulator

This change:
1. Adds NavigationSimulator::SetMethod to set the initial HTTP method.

2. Converts navigation intercept tests to use the nav simulator. Note
   well that this conversion *relaxes* the test constraints. We now
   simply check that the request fails to commit, rather than fails
   to start. This is to allow future optimizations to apply policy
   in an async way without blocking the majority of network requests.
   For POSTs, we still check that the nav is cancelled at start.

3. Converts flash download intercept test to use the nav simulator.

Bug: 793053

Change-Id: Ic30bf123fa94abe978ca4f5b02802899bc9a423e
Reviewed-on: https://chromium-review.googlesource.com/813417
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527095}
[modify] https://crrev.com/003ae0aebb04d05bf187b8dbfcfdb165f78952a3/chrome/browser/plugins/flash_download_interception_unittest.cc
[modify] https://crrev.com/003ae0aebb04d05bf187b8dbfcfdb165f78952a3/components/navigation_interception/intercept_navigation_throttle_unittest.cc
[modify] https://crrev.com/003ae0aebb04d05bf187b8dbfcfdb165f78952a3/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/003ae0aebb04d05bf187b8dbfcfdb165f78952a3/content/public/test/navigation_simulator.h

Might get to this...

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d2400e352ab65ebd1cd6af360f1a8e9af525bf1

commit 0d2400e352ab65ebd1cd6af360f1a8e9af525bf1
Author: Charlie Harrison <csharrison@chromium.org>
Date: Mon Jun 04 18:52:48 2018

Use NavigationSimulator in RecentTabHelperTest

NavigationSimulator supports throttles which can defer the navigation.
If any throttle is added by default on Android which defers a navigation,
these tests will fail.

This CL also cleans up some unused #includes.

Bug: 793053
Change-Id: I1875f189a2ed22ae40f75a405e7c6c7fc989efc2
Reviewed-on: https://chromium-review.googlesource.com/1083892
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564169}
[modify] https://crrev.com/0d2400e352ab65ebd1cd6af360f1a8e9af525bf1/chrome/browser/offline_pages/recent_tab_helper_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00

commit ffaf1e2ba66b521c4afe3af865a3f044ae1bff00
Author: Charlie Harrison <csharrison@chromium.org>
Date: Fri Jun 08 22:46:08 2018

Make the intercept navigation throttle async

Design doc: https://bit.ly/2JgkZyc

This CL changes the intercept navigation throttle to avoid blocking
network requests from starting. Instead, policy decisions are computed
asynchronously, and the load is canceled+ignored at the next
NavigationThrottle callback.

Note that in cases where the request should be ignored, this wastes
some bytes. However, this is a pretty rare case. It looks like ~99% of
the time we won't cancel the navigation (from
Android.TabNavigationInterceptResult)

Bug: 793053
Change-Id: I9bf4eb29cf7ff92d31432eedb41cd446f738a492
Reviewed-on: https://chromium-review.googlesource.com/815354
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565764}
[modify] https://crrev.com/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00/components/navigation_interception/BUILD.gn
[modify] https://crrev.com/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00/components/navigation_interception/intercept_navigation_throttle.cc
[modify] https://crrev.com/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00/components/navigation_interception/intercept_navigation_throttle.h
[modify] https://crrev.com/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00/components/navigation_interception/intercept_navigation_throttle_unittest.cc
[modify] https://crrev.com/ffaf1e2ba66b521c4afe3af865a3f044ae1bff00/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/339bb74d7cbff8a708fc043180ed0e46c389b7e6

commit 339bb74d7cbff8a708fc043180ed0e46c389b7e6
Author: Charlie Harrison <csharrison@chromium.org>
Date: Wed Jun 20 14:15:54 2018

Add AsyncNavigationIntercept to fieldtrial_testing_config

Bug: 793053
Change-Id: I0d39cd8e6695b18da47549e632f10abd9919c83e
Reviewed-on: https://chromium-review.googlesource.com/1106519
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568829}
[modify] https://crrev.com/339bb74d7cbff8a708fc043180ed0e46c389b7e6/testing/variations/fieldtrial_testing_config.json