Service Worker: Delay stale resource cleanup

Service Worker: Delay stale resource cleanup

Before this patch, we eagerly cleaned up purgeable resources when the
ServiceWorkerStorage object is initialized, which could result in an IO
flurry at browser startup. This patch:
- Cleans up uncommitted resources (not just purgeable resources) left
over from the last browser session
- Performs that cleanup lazily, when a mutation of the
purgeable or uncommitted resources is about to occur

BUG=388095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282035

[falken, 2014-07-08 12:24:24]

PTAL

[michaeln, 2014-07-09 02:36:11]

lgtm

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:976:
BumpNextResourceIdIfNeeded(ids, batch);
lions and tigers and bears, thnx for fixing this

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:539: state_ =
HAS_STALE_RESOURCES;
we don't really know if we have stale resources yet, we know we have to check,
is that right?

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:977:
ServiceWorkerStorage::CreateResponseReader(int64 response_id) {
i think this accidentally got move far away from its sibling
CreateResponseWriter method?

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:338:
HAS_STALE_RESOURCES,
bikesheddy...

This one is a little different then the other states. Would this be better as a
flag, has_checked_for_stale_resources_? If we keep it a state, having
INITIALIZED in both of the names might make sense

INITIALIZED_FOR_READING
INITIALIZED_FOR_WRITING

[nhiroki, 2014-07-09 03:43:10]

lgtm

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:976:
BumpNextResourceIdIfNeeded(ids, batch);
Great catch! Thank you :)

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:1063: for
(std::set<int64>::const_iterator itr = used_ids.begin();
IIUC, std::set is always sorted and the last element should be the biggest one.

[falken, 2014-07-09 04:31:14]

Thanks for the reviews.

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:1063: for
(std::set<int64>::const_iterator itr = used_ids.begin();
On 2014/07/09 03:43:10, nhiroki wrote:
> IIUC, std::set is always sorted and the last element should be the biggest
one.

Ah, looks right. Switched to look at the last element.

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:539: state_ =
HAS_STALE_RESOURCES;
On 2014/07/09 02:36:11, michaeln wrote:
> we don't really know if we have stale resources yet, we know we have to check,
> is that right?

correct, not the best name

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:977:
ServiceWorkerStorage::CreateResponseReader(int64 response_id) {
On 2014/07/09 02:36:11, michaeln wrote:
> i think this accidentally got move far away from its sibling
> CreateResponseWriter method?

oops yes, done

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/374873002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:338:
HAS_STALE_RESOURCES,
On 2014/07/09 02:36:11, michaeln wrote:
> bikesheddy...
> 
> This one is a little different then the other states. Would this be better as
a
> flag, has_checked_for_stale_resources_? If we keep it a state, having
> INITIALIZED in both of the names might make sense
> 
> INITIALIZED_FOR_READING
> INITIALIZED_FOR_WRITING

I was a bit torn when writing this. I liked State because it constrains the
possible states instead of a combinatoric explosion of flags and states. But it
does seem to make the code more convoluted with the introduction of IsReady. I
switched back to flag, it feels nicer.

[falken, 2014-07-09 04:33:35]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-07-09 04:37:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/374873002/60001

[commit-bot: I haz the power, 2014-07-09 05:25:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26300)

[commit-bot: I haz the power, 2014-07-09 05:27:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-09 05:28:00]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26314)

[falken, 2014-07-09 05:28:58]

The CQ bit was unchecked by falken@chromium.org

[falken, 2014-07-09 05:29:25]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-07-09 05:30:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/374873002/80001

[commit-bot: I haz the power, 2014-07-09 11:03:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-09 14:39:34]

Change committed as 282035