Consider increasing cache_storage TaskPriority to USER_VISIBLE from BEST_EFFORT |
||
Issue descriptionCurrently the cache_storage backend uses a SequenceTaskRunner with TaskPriority::BEST_EFFORT: https://cs.chromium.org/chromium/src/content/browser/cache_storage/cache_storage_context_impl.cc?l=37-40&rcl=1ad8e3411d765440af317eda4f5715d232b00317 If I'm reading the TaskScheduler code correctly that schedules cache query operations at the lowest priority. In addition, the total number of threads across the browser permitted for this priority seems to be 1 or 2. Also, I believe that if there are many higher priority tasks then BEST_EFFORT tasks may be arbitrarily delayed. In contrast, other storage APIs like IndexedDB pass TaskPriority::USER_VISIBLE: https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_context_impl.cc?l=116-119&rcl=abc915422e0d17fd1f8dda95b42d18b75b4159e0 Also, perhaps more importantly, reading Blob data uses USER_VISIBLE as well: https://cs.chromium.org/chromium/src/storage/browser/blob/blob_reader.cc?l=76-77&rcl=abc915422e0d17fd1f8dda95b42d18b75b4159e0 This is potentially bad news for sites using service workers to offline their content. If the SW goes to Cache API for many requests in a short period you could end up with early response body reading starving out later cache.match() calls. Given that service workers must round trip the cache.match() result to the script before the body can start to be read this may add significant delay to the overall loading process. Of course, that is all theory and I need to investigate the impact of changing the priority.
,
Aug 13
,
Aug 14
I think running at the lowest priority is clearly a mistake. I'm just going to update the value since that is the likely answer no matter what benchmarks I construct. It will probably only have an impact on stressed/low-end devices which may be hard to reproduce in my current setup here.
,
Aug 14
,
Aug 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a668bf90e7255e73726dcc27994c86fed94b422d commit a668bf90e7255e73726dcc27994c86fed94b422d Author: Ben Kelly <wanderview@chromium.org> Date: Tue Aug 14 16:35:56 2018 Execute cache_storage queries with TaskPriority::USER_VISIBLE. Cache API performance is visible to the user via page load times when service workers are in use. In addition, this value is consistent with the TaskPriority used by other storage APIs. R=jsbell@chromium.org Bug: 873771 Change-Id: I831ad645ad8adc92618f5565789103abd162ca98 Reviewed-on: https://chromium-review.googlesource.com/1174652 Reviewed-by: Joshua Bell <jsbell@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/heads/master@{#582938} [modify] https://crrev.com/a668bf90e7255e73726dcc27994c86fed94b422d/content/browser/cache_storage/cache_storage_context_impl.cc
,
Aug 14
|
||
►
Sign in to add a comment |
||
Comment 1 by wanderview@chromium.org
, Aug 13