New issue
Advanced search Search tips

Issue 873771 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 14
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 667892
issue 853518



Sign in to add a comment

Consider increasing cache_storage TaskPriority to USER_VISIBLE from BEST_EFFORT

Project Member Reported by wanderview@chromium.org, Aug 13

Issue description

Currently 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.
 
Alternatively, if this just makes sense in general we could make the change and measure any impact.
Blocking: 667892
It looks like this low priority was added in an automated CL in  issue 667892 .
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.
Blocking: 853518
This possibly helps with issue 853518.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Labels: M-70
Status: Fixed (was: Assigned)

Sign in to add a comment