Priority inversion reading cookies on startup/session-restore/navigation (?) |
||||||
Issue descriptionChrome Version: M70 (and many prior) OS: All What steps will reproduce the problem? (1) Navigate while machine is under stress (e.g. large session restore) What is the expected result? Reasonable performance What happens instead? Long delays Looking at a startup (session restore) trace one thing that jumps out is the slow communication with the cookie store's on-disk backend: src_file "../../net/extras/sqlite/sqlite_persistent_cookie_store.cc" src_func "LoadCookiesForKey" / "Load" These are happening at TaskPriority::BEST_EFFORT and that seems wrong to me? Is navigation blocked on these? (if so this is definitely a blocker for issue 831835) (see SS -- full trace @ https://drive.google.com/open?id=1TPKGIr25mEidn_JVbPXUSPWnzS9dRnJK) I'm not sure exactly which impl this corresponds to in practice but it's either : 1) https://cs.chromium.org/chromium/src/content/browser/net/quota_policy_cookie_store.cc?sq=package:chromium&dr&q=file:quota_policy_cookie_store.cc++base::TaskPriority::BEST_EFFORT&g=0&l=91 or 2) https://cs.chromium.org/chromium/src/services/network/network_context.cc?type=cs&q=file:network_context.cc+base::TaskPriority::BEST_EFFORT&g=0&l=993
,
Aug 28
@etienneb will be taking a look. Let's prioritize this for M70 (branches very soon)
,
Aug 28
Yes, loading data from the cookie store can block any network request that has cookies. We don't need to load the entire store before we can start sending requests, but we do need to load individual cookies. I'm not sure why this was set to background - it's not blocking anything when we start loading it, but soon after it does block activity.
,
Sep 12
,
Sep 14
Well, some of loading is high-priority, but write back is very much background priority, so ideally one would want to change it dynamically somehow --- something like a downgrade to background after loading everything would probably be the simplest usable form.
,
Sep 14
Do we ever need to load more things after startup? i.e. after beginning to write back? Can write backs be reordered? We are thinking of adding a way to toggle priority of an existing sequence but we can only safely degrade (USER_BLOCKING=>BEST_EFFORT); an upgrade (BEST_EFFORT=>USER_BLOCKING) risks potentially having to flush a long unprocessed queue before getting to the important work. If the work can be reordered we are also thinking of exposing "sequence funneling" abilities where mutually exclusive but otherwise reorderable best effort tasks could use a "child sequence" and allow important work to get ahead of it.
,
Sep 14
There can be some mixing of load ops and write ops, though it's pretty uncommon, I think, and there is a point after which no additional load ops can ever happen, so a priority downgrade would be possible to issue there, and seems like would be a clear improvement over the current state. Actually only some of load ops are critical --- others are meant to gradually load in the cookie jar in background, so in that sense the reordering-type solution may be even better, but I can't say if it would be safe off the top of my head; it may be possible since higher-level stuff wouldn't let one write to rows that haven't been loaded in yet, but there is enough stuff going on that it would take careful code study. P.S. I would love the priority downgrade knob for SimpleCache's index as well, though that would probably just introduce priority inversion for some kinds of caches...
,
Sep 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6 commit 45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6 Author: Francois Doray <fdoray@chromium.org> Date: Mon Sep 17 19:35:00 2018 net: Make net:extras a component instead of a static library. net:extras is currently linked into multiple shared libraries used by Chrome in a component build. That prevents adding a base::Feature member into a net:extras source file, because there are runtime checks that a feature is only defined once. This CL makes net:extras a component rather than a static library. This ensure that there is only one copy of net:extras in memory when running a Chrome component build. Bug: 878222 Change-Id: I9f6c87171f7fc68f3f1e62b879f8387f3985f4f8 Reviewed-on: https://chromium-review.googlesource.com/1225308 Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#591781} [modify] https://crrev.com/45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6/net/BUILD.gn [modify] https://crrev.com/45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6/net/extras/sqlite/cookie_crypto_delegate.h [modify] https://crrev.com/45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6/net/extras/sqlite/sqlite_channel_id_store.h [modify] https://crrev.com/45bdc4c3cc0e47e380b1ff636f5ffe0dfd1d24d6/net/extras/sqlite/sqlite_persistent_cookie_store.h
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/87bb331bd43bae02932037e48cfd6e9343122b43 commit 87bb331bd43bae02932037e48cfd6e9343122b43 Author: Etienne Pierre-Doray <etiennep@chromium.org> Date: Tue Sep 18 17:27:12 2018 [TaskScheduler]: Fix priority inversion reading cookies. This CL creates a feature kCookieStorePriorityBoost that changes SQLitePersistentCookieStore background_task_runner sequence priority to USER_BLOCKING. A CANARY_DEV_50 experiment will be launch and used to compare SessionRestore.ForegroundTabFirstPaint3. Bug: 878222 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I22bbd5b90cf4d36d3a858adbe94eff96f280a5a0 Reviewed-on: https://chromium-review.googlesource.com/1224671 Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org> Reviewed-by: Maks Orlovich <morlovich@chromium.org> Reviewed-by: Jialiu Lin <jialiul@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#592094} [modify] https://crrev.com/87bb331bd43bae02932037e48cfd6e9343122b43/components/safe_browsing/browser/safe_browsing_url_request_context_getter.cc [modify] https://crrev.com/87bb331bd43bae02932037e48cfd6e9343122b43/content/browser/net/quota_policy_cookie_store.cc [modify] https://crrev.com/87bb331bd43bae02932037e48cfd6e9343122b43/net/extras/sqlite/sqlite_persistent_cookie_store.cc [modify] https://crrev.com/87bb331bd43bae02932037e48cfd6e9343122b43/net/extras/sqlite/sqlite_persistent_cookie_store.h [modify] https://crrev.com/87bb331bd43bae02932037e48cfd6e9343122b43/services/network/network_context.cc
,
Sep 21
FWIW, Cookie.PriorityBlockingTime and Cookie.TimeBlockedOnLoad might be of some interest (but less important than whole-Chrome metrics, of course)
,
Sep 27
Got some finch results [1]. Cookie.PriorityBlockingTime and Cookie.TimeBlockedOnLoad are definitely improved (expected), by as much as 50% on 50th percentile. However, I don't see as much difference in relevant heartbeat metrics as expected (SessionRestore.ForegroundTabFirstPaint3). Improvement of 4% on 99th percentile is not considered significant.. For some reason, Browser and Rendered memory footprint were improved, though I'm not sure that's related. Some metrics were also slightly regressed (CharTypedToRepaintLatency, NonEmptyPaint2). Should we go forward and merge this / experiment on stable? [1]: https://uma.googleplex.com/p/chrome/variations/?sid=718c47204b5f5d8cf14ec702c9e7733b
,
Sep 27
No we shouldn't bring performance experiments with unclear conclusions to Stable. I'll try locally to see if it helps the bad session restore case I'm seeing. The problem with metrics on session restore is that it's an infrequent event (especially large session restores), most users don't even do 1 per day. We can try to see the Timeline dashboard @ 99.9th percentile and add a "split by" filter for your experiment (but again, sample size might be an issue) Le jeu. 27 sept. 2018 11 h 44, etiennep via monorail < monorail+v2.527760868@chromium.org> a écrit :
,
Oct 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7b910c700ce7be682dd09cb0cab842f01e56ca76 commit 7b910c700ce7be682dd09cb0cab842f01e56ca76 Author: Francois Doray <fdoray@chromium.org> Date: Sat Oct 27 04:21:28 2018 Use USER_BLOCKING priority in SQLitePersistentCookieStore. We have verified that tasks posted by SQLitePersistentCookieStore are on the critical path of a page load. According to the documentation in task_traits.h, being on the critical path of a page load requires USER_BLOCKING priority. We keep this as an experiment because we can to assess the impact of this change when the WindowsThreadModeBackground feature is enabled. Bug: 872820, 878222 Change-Id: Iec368de8f59b34703688d8f8811aedbeddc50a8d Reviewed-on: https://chromium-review.googlesource.com/c/1303039 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#603308} [modify] https://crrev.com/7b910c700ce7be682dd09cb0cab842f01e56ca76/net/extras/sqlite/sqlite_persistent_cookie_store.cc
,
Jan 4
What's the status of this?
,
Jan 7
CookieStorePriorityBoost is now enabled by default, so SQLitePersistentCookieStore always run with priority USER_BLOCKING, because it was identified on the critical path of a page load. I'm marking the bug as fixed and cleaning up the experiment.
,
Jan 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8c0fca2ac2d609e1963e311ebd5a832d98b8d96 commit d8c0fca2ac2d609e1963e311ebd5a832d98b8d96 Author: Etienne Pierre-doray <etiennep@chromium.org> Date: Mon Jan 07 19:51:30 2019 Cleanup CookieStorePriority experiment. SQLitePersistentCookieStore priority is now defaulted to USER_BLOCKING. Bug: 872820, 878222 Change-Id: Ie5c062b6718082534d61725601b7884ab29d9b49 Reviewed-on: https://chromium-review.googlesource.com/c/1398062 Commit-Queue: Victor Costan <pwnall@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#620433} [modify] https://crrev.com/d8c0fca2ac2d609e1963e311ebd5a832d98b8d96/net/extras/sqlite/sqlite_persistent_cookie_store.cc |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by gab@chromium.org
, Aug 28