Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950533002:
android: Disable the startup DNS resolutions. (Closed)
Descriptionandroid: Disable the startup DNS resolutions.
On all platforms, Chrome observes the first requests at startup and
speculatively starts DNS resolution for these at the next startup. It
has limited effect on Android, as subsequent startups are likely to be
different, due to the lack of homepage and pinned tabs.
Furthermore, it can tie up to 3 threads on the WorkerPool thread pool,
as DNS resolution is synchronous on Android. This thread pool is also
used for certificate validation for instance.
BUG=699080
Review-Url: https://codereview.chromium.org/2950533002
Cr-Commit-Position: refs/heads/master@{#482583}
Committed: https://chromium.googlesource.com/chromium/src/+/8e8aa98f3b4b67412dc9d2a842d457f40b12caad
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 3
Patch Set 5 : Remove dead code. #
Total comments: 2
Patch Set 6 : Rebase. #Patch Set 7 : nit. #Messages
Total messages: 33 (19 generated)
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== android: Disable the startup DNS resolutions. On all platforms, Chrome observes the first requests at startup and speculatively starts DNS resolution for these at the next startup. It has limited effect on Android, as subsequent startups are likely to be different, due to the lack of homepage and pinned tabs. Furthermore, it can tie up to 3 threads on the WorkerPool thread pool, as DNS resolution is synchronous on Android. This thread pool is also used for certificate validation for instance. ========== to ========== android: Disable the startup DNS resolutions. On all platforms, Chrome observes the first requests at startup and speculatively starts DNS resolution for these at the next startup. It has limited effect on Android, as subsequent startups are likely to be different, due to the lack of homepage and pinned tabs. Furthermore, it can tie up to 3 threads on the WorkerPool thread pool, as DNS resolution is synchronous on Android. This thread pool is also used for certificate validation for instance. BUG=699080 ==========
lizeb@chromium.org changed reviewers: + csharrison@chromium.org
Are the assumptions around this CL likely to change in the future? If we think this will be permanent (e.g. on Android DNS prefetching at startup will never really be worth it), we should also stop observing / persisting startup URLs to prefetch, since afaict this is their only use. This might further improve startup because we could remove these from prefs.
Thanks for the comment, you're right. Updated to disable the observer on Android. This leaves a bit of dead code behind and a single unique_ptr<>, but the alternatives seemed to be a bit of dead code or quite a few #ifdefs. Seems simpler this way, and hopefully the constexpr config is enough for the compiler to eliminate some code in the branches that are not taken. I didn't want to refactor the code too much as it is now. wdyt?
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Code wise this LGTM, but I'm wondering if we have a way of validating the hypothesis? Do we expect startup time to go down in histograms, or have you done a lab test? It might be too much overhead for a small experiment in canary/dev.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 12:24:13, on gerrit - ping aggressively wrote: > Code wise this LGTM, but I'm wondering if we have a way of validating the > hypothesis? > > Do we expect startup time to go down in histograms, or have you done a lab test? > It might be too much overhead for a small experiment in canary/dev. I've done local tracing. From my real profile, the set of domains is mostly irrelevant, since it pertains to the previous Chrome start. There are a couple more issues as well: - The set of domains in collected from a ResourceThrottle in WillStartRequest, so we don't even know whether the DNS records were needed - If we end up loading the same page we did last time, the TabHelper will trigger the preconnection and pre-resolution a few ms later, in which case we're likely to re-issue the same DNS requests - On Wifi, I see random crazy latency spikes for DNS requests (maybe packet loss), and a DNS request can randomly take 1+s even though it's usually fast. From traces on a normal profile, the deserialization can take ~40-60ms on the IO thread. So the upshot is to remove a task from the IO thread (IO thread timings are to be taken with a grain of salt, as it is quite often descheduled at that stage during startup), and avoid bad cases where we block threads on this thread pool for no good reason. Eventually, we should probably replace that with "10 domains you're really likely to always connect to", but that's not what it does right now.
Thanks for the info. 40-60ms deserialization time is much worse than I expected, even with descheds. https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:30: #include "base/trace_event/trace_event.h" Is it used?
https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:30: #include "base/trace_event/trace_event.h" On 2017/06/20 13:24:29, on gerrit - ping aggressively wrote: > Is it used? Yes, there is a TRACE_EVENT0() added below.
FYI I am not an owner here so we'll need chrome/browser/net signoff too. https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2950533002/diff/60001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:30: #include "base/trace_event/trace_event.h" On 2017/06/20 13:30:17, Benoit L wrote: > On 2017/06/20 13:24:29, on gerrit - ping aggressively wrote: > > Is it used? > > Yes, there is a TRACE_EVENT0() added below. Ahh sorry. It's early :)
Thanks! I knew that you're not an owner, but wanted your opinion first. I've updated the CL to remove the observer from the .h, and checked that with the constexpr conditions, InitialObserver is stripped from the release build, that is no dead code. PTAL, thanks!
The CQ bit was checked by lizeb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lizeb@chromium.org changed reviewers: + jkarlin@chromium.org
+jkarlin for review and OWNERS approval as well. PTAL, thanks!
lgtm with nit https://codereview.chromium.org/2950533002/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2950533002/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:757: // May be empty if kInitialDnsPrefetchListEnabled is false. Still update the New line above this
Thanks! https://codereview.chromium.org/2950533002/diff/80001/chrome/browser/net/pred... File chrome/browser/net/predictor.cc (right): https://codereview.chromium.org/2950533002/diff/80001/chrome/browser/net/pred... chrome/browser/net/predictor.cc:757: // May be empty if kInitialDnsPrefetchListEnabled is false. Still update the On 2017/06/26 16:47:56, jkarlin_slow wrote: > New line above this Done.
The CQ bit was checked by lizeb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2950533002/#ps120001 (title: "nit.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498552681202590,
"parent_rev": "fd14deaa64e224be52fd0bf77ef3f5a450404da5", "commit_rev":
"8e8aa98f3b4b67412dc9d2a842d457f40b12caad"}
Message was sent while issue was closed.
Description was changed from ========== android: Disable the startup DNS resolutions. On all platforms, Chrome observes the first requests at startup and speculatively starts DNS resolution for these at the next startup. It has limited effect on Android, as subsequent startups are likely to be different, due to the lack of homepage and pinned tabs. Furthermore, it can tie up to 3 threads on the WorkerPool thread pool, as DNS resolution is synchronous on Android. This thread pool is also used for certificate validation for instance. BUG=699080 ========== to ========== android: Disable the startup DNS resolutions. On all platforms, Chrome observes the first requests at startup and speculatively starts DNS resolution for these at the next startup. It has limited effect on Android, as subsequent startups are likely to be different, due to the lack of homepage and pinned tabs. Furthermore, it can tie up to 3 threads on the WorkerPool thread pool, as DNS resolution is synchronous on Android. This thread pool is also used for certificate validation for instance. BUG=699080 Review-Url: https://codereview.chromium.org/2950533002 Cr-Commit-Position: refs/heads/master@{#482583} Committed: https://chromium.googlesource.com/chromium/src/+/8e8aa98f3b4b67412dc9d2a842d4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/8e8aa98f3b4b67412dc9d2a842d4... |
