Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152543002:
ServiceWorker: Migrate the script cache backend from BlockFile to Simple (Closed)
|
Created:
5 years, 7 months ago by nhiroki Modified:
5 years, 6 months ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, falken, yhirano Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Migrate the script cache backend from BlockFile to Simple
This CL switches the backend of ServiceWorkerDiskCache from BlockFile to Simple,
and migrates cached resources before initializing the diskcache.
* DiskCache migration consists of 2 parts:
1) Copying contents into a new diskcache. This should be complete before
starting diskcache initialization.
2) Lazily removing an old diskcache after migration is complete.
* These migration states are recorded in ServiceWorkerDatabase. If migration
is aborted in the middle of the execution, the retry will happen on the next
initialization.
* If migration fails, DeleteAndStartOver runs and creates a new storage.
BUG=487482
TEST=content_unittests --gtest_filter=ServiceWorker*
TEST=content_browsertests --gtest_filter=ServiceWorker*
TEST=manual (run Chromium with an old diskcache and make sure migration correctly happens)
Committed: https://crrev.com/d1c9f9fbe26992894355b68f0445e03c2061908f
Cr-Commit-Position: refs/heads/master@{#334811}
Patch Set 1 : #Patch Set 2 : add uma and error handling (still needs to update histograms.xml) #Patch Set 3 : update histograms.xml #
Total comments: 14
Patch Set 4 : address review comments #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : might fix win builds #Patch Set 9 : check the old diskcache directory in ReadInitialDataFromDB #Patch Set 10 : clean up #Patch Set 11 : delete old disk cache directory on AbortLazyInitialize #Patch Set 12 : [NOT READY FOR REVIEW] lazily migrate diskcache #Patch Set 13 : update tests #
Total comments: 8
Patch Set 14 : rebase #Patch Set 15 : incorporate falken@'s comments #
Total comments: 16
Patch Set 16 : move diskcache initialization from storage to migrator #Patch Set 17 : NOT READY FOR REVIEW #Patch Set 18 : introduce purgeable files #Patch Set 19 : remove needs_disk_cache_migration #
Total comments: 18
Patch Set 20 : rebase on https://codereview.chromium.org/1181573002/ #Patch Set 21 : remake #
Total comments: 19
Patch Set 22 : rebase #Patch Set 23 : use PostAfterStartupTask #
Total comments: 4
Patch Set 24 : IsDiskCacheMigrationNeeded() and IsOldDiskCacheDeletionNeeded() #Patch Set 25 : fix rebase failure #
Total comments: 8
Patch Set 26 : update comment #
Total comments: 12
Patch Set 27 : address falken@'s comments #
Total comments: 2
Patch Set 28 : UMA_HISTOGRAM_COUNTS #Patch Set 29 : support the case that the old diskcache exists but the database does not #
Total comments: 2
Patch Set 30 : !defined(NDEBUG) #Patch Set 31 : remove defined(UNIT_TEST) #Patch Set 32 : rebase #Messages
Total messages: 88 (35 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
nhiroki@chromium.org changed reviewers: + kinuko@chromium.org, michaeln@chromium.org
Hi michaeln@ and kinuko@, Can you give me your early feedback on this? This is still a bit rough and should have UMAs, but a basic part would be almost complete. Thanks!
+falken@ to CC
Patchset #3 (id:260001) has been deleted
This is now ready for review. Removed WIP mark. Can you take a look? Thanks!
falken@chromium.org changed reviewers: + falken@chromium.org
Looks like a good approach and code to me. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:288: Should we also do a test for if the migration fails? https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1072: ServiceWorkerMetrics::DiskCacheMigrationResult result) { DCHECK(result != OK)? https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1080: RunSoon(FROM_HERE, pending_task); Is this OK? Should the pending tasks be run after start over is complete? I actually don't really understand the comment: is it saying "abort the pending tasks" or "when the pending tasks run they will be aborted" or "if we don't run them now, the pending tasks would be aborted"...? https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:299: base::FilePath GetOldDiskCachePath(); Can you add a comment about what the Old* functions are for, and when we plan to remove them? https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:307: void DidDiskCacheMigrated(scoped_ptr<ServiceWorkerDiskCacheMigrator> migrator, nit: this is ungrammatical... maybe DidMigrateDiskCache?
(Haven't looked at the all files, but sending some comments) https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:278: LazyInitialize(); Is it possible to also test that concurrent requests made during LazyInitialize are delayed until the migration's done? https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:278: LazyInitialize(); Did you check the local histogram to see how long it took?
Patchset #4 (id:300001) has been deleted
Thank you for reviewing! Updated. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:278: LazyInitialize(); On 2015/06/02 06:18:52, kinuko wrote: > Is it possible to also test that concurrent requests made during LazyInitialize > are delayed until the migration's done? Done. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:278: LazyInitialize(); On 2015/06/02 06:18:52, kinuko wrote: > Did you check the local histogram to see how long it took? I tried the migration 3 times (Linux, SSD, Release build) and checked its local histograms: - NumberOfMigratedResources: 4 - MigrationTime: 7 msec, 7 msec, 8 msec Instructions: (1) Build chromium without this patch (2) Browse Trained-to-Thrill, SVGOMG and SimplePushDemo (4 resources are cached) (3) Build chromium with this patch (4) Open chromium and start the migration (5) Check "chrome://histograms" https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:288: On 2015/06/01 04:31:49, falken wrote: > Should we also do a test for if the migration fails? I haven't found a handy way to fail the migration yet... (still investigating) https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1072: ServiceWorkerMetrics::DiskCacheMigrationResult result) { On 2015/06/01 04:31:49, falken wrote: > DCHECK(result != OK)? Done. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1080: RunSoon(FROM_HERE, pending_task); On 2015/06/01 04:31:50, falken wrote: > Is this OK? Should the pending tasks be run after start over is complete? > > I actually don't really understand the comment: is it saying "abort the pending > tasks" or "when the pending tasks run they will be aborted" or "if we don't run > them now, the pending tasks would be aborted"...? ScheduleDeleteAndStartOver() disables the storage and schedules the recovery task to be accompanied with multiple thread hops, so the pending tasks should run before the recovery is done and fail due to the disabled storage. Updated the comment. Please let me know if this is still ambiguous. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:299: base::FilePath GetOldDiskCachePath(); On 2015/06/01 04:31:50, falken wrote: > Can you add a comment about what the Old* functions are for, and when we plan to > remove them? Done. https://codereview.chromium.org/1152543002/diff/280001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:307: void DidDiskCacheMigrated(scoped_ptr<ServiceWorkerDiskCacheMigrator> migrator, On 2015/06/01 04:31:50, falken wrote: > nit: this is ungrammatical... maybe DidMigrateDiskCache? Done.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152543002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
i didn't get to this today, but i'll definitely take a look at this tomorrow
Gating first page load on migration completion has been bugging me. And contributing additional delays during normal startups maybe moreso. I have some goals and some ideas to meet those goals to propose. Goals 1) Once migration is complete, the new startup sequence should incur no more cost than the existing startup sequence. 2) Migration should not block users that are not visiting sw enabled sites. Only migrate when needed. I think i see how that those goals can be met w/o adding complexity. **** To accomplish 1: Store whether migration has occurred in the sw database as part of the initial data. Maybe two additional bools: data_migrated_, old_data_deleted_. In the expected case, there are no additional tasks to be posted, queued, or run compared to what we have now. **** To accomplish 2: Two parts: Part A) Defer migration until ServiceWorkerStorage::disk_cache() is accessed, disk_cache() access is deferred until we need to read or write something from the script cache. Start up a migrator then. PartB) Once accessed, defer the start of initialization of the instance returned until migration is complete. Take advantage of the lazy init mechanism in the AppCacheDiskCache. That class has a queueing mechanism and already knows how to wait until init completion prior to going live and callers of the DiskCache are fine with waiting as long as needed. So replace AppCacheDiskCache::is_initializing() with :::is_initializing_or_waiting_to_initialize(). When migration is complete, start initializing the instance thats been waiting and return to business as usual. I don't think that adds any complexity really, but it does change when some things occur in ways that are advantageous. wdyt?
On 2015/06/05 01:16:15, michaeln wrote: > Gating first page load on migration completion has been bugging me. And > contributing additional delays during normal startups maybe moreso. I have some > goals and some ideas to meet those goals to propose. > > Goals > > 1) Once migration is complete, the new startup sequence should incur no more > cost than the existing startup sequence. > > 2) Migration should not block users that are not visiting sw enabled sites. Only > migrate when needed. > > I think i see how that those goals can be met w/o adding complexity. > > **** To accomplish 1: > > Store whether migration has occurred in the sw database as part of the initial > data. Maybe two additional bools: data_migrated_, old_data_deleted_. In the > expected case, there are no additional tasks to be posted, queued, or run > compared to what we have now. Or could we do the dir check in the same task as the db init? > **** To accomplish 2: > > Two parts: > > Part A) Defer migration until ServiceWorkerStorage::disk_cache() is accessed, > disk_cache() access is deferred until we need to read or write something from > the script cache. Start up a migrator then. Given that NTP is considering going out of experiment I suspect this may not change the sequence a lot? (I remember we were considering doing this in disk_cache though, haven't checked in why we didn't do so) > PartB) Once accessed, defer the start of initialization of the instance returned > until migration is complete. Take advantage of the lazy init mechanism in the > AppCacheDiskCache. That class has a queueing mechanism and already knows how to > wait until init completion prior to going live and callers of the DiskCache are > fine with waiting as long as needed. So replace > AppCacheDiskCache::is_initializing() with > :::is_initializing_or_waiting_to_initialize(). > > When migration is complete, start initializing the instance thats been waiting > and return to business as usual. > > I don't think that adds any complexity really, but it does change when some > things occur in ways that are advantageous. > > wdyt? All sounds good, basically.
Thank you for reviewing! Michael's approach looks great. I'll work on it. On 2015/06/05 01:42:03, kinuko wrote: > On 2015/06/05 01:16:15, michaeln wrote: > > Gating first page load on migration completion has been bugging me. And > > contributing additional delays during normal startups maybe moreso. I have > some > > goals and some ideas to meet those goals to propose. > > > > Goals > > > > 1) Once migration is complete, the new startup sequence should incur no more > > cost than the existing startup sequence. > > > > 2) Migration should not block users that are not visiting sw enabled sites. > Only > > migrate when needed. > > > > I think i see how that those goals can be met w/o adding complexity. > > > > **** To accomplish 1: > > > > Store whether migration has occurred in the sw database as part of the initial > > data. Maybe two additional bools: data_migrated_, old_data_deleted_. In the > > expected case, there are no additional tasks to be posted, queued, or run > > compared to what we have now. > > Or could we do the dir check in the same task as the db init? Checking in the same task would be feasible. Additional fields in the database could become debt... so I'd prefer to choose this way if possible. > > **** To accomplish 2: > > > > Two parts: > > > > Part A) Defer migration until ServiceWorkerStorage::disk_cache() is accessed, > > disk_cache() access is deferred until we need to read or write something from > > the script cache. Start up a migrator then. > > Given that NTP is considering going out of experiment I suspect this may not > change the sequence a lot? > > (I remember we were considering doing this in disk_cache though, haven't checked > in why we didn't do so) Yes, we considered this before but gave it up because we thought DiskCacheReader/Writer could interleave migration tasks with regular read/write operations. But, according to the Part B, disk_cache has a queuing mechanism and this seems to be able to work fine. > > PartB) Once accessed, defer the start of initialization of the instance > returned > > until migration is complete. Take advantage of the lazy init mechanism in the > > AppCacheDiskCache. That class has a queueing mechanism and already knows how > to > > wait until init completion prior to going live and callers of the DiskCache > are > > fine with waiting as long as needed. So replace > > AppCacheDiskCache::is_initializing() with > > :::is_initializing_or_waiting_to_initialize(). > > > > When migration is complete, start initializing the instance thats been waiting > > and return to business as usual. > > > > I don't think that adds any complexity really, but it does change when some > > things occur in ways that are advantageous. > > > > wdyt? > > All sounds good, basically. SGTM, too.
> > > **** To accomplish 2: > > > > > > Two parts: > > > > > > Part A) Defer migration until ServiceWorkerStorage::disk_cache() is > accessed, > > > disk_cache() access is deferred until we need to read or write something > from > > > the script cache. Start up a migrator then. > > > > Given that NTP is considering going out of experiment I suspect this may not > > change the sequence a lot? > > > > (I remember we were considering doing this in disk_cache though, haven't > checked > > in why we didn't do so) > > Yes, we considered this before but gave it up because we thought > DiskCacheReader/Writer could interleave migration tasks with regular read/write > operations. But, according to the Part B, disk_cache has a queuing mechanism and > this seems to be able to work fine. Ah, I remember one more thing! We cannot do the migration before the diskcache is initialized because the migrator uses the ResponseReader/Writer... hmmm...
On 2015/06/05 03:18:28, nhiroki wrote: > > > > **** To accomplish 2: > > > > > > > > Two parts: > > > > > > > > Part A) Defer migration until ServiceWorkerStorage::disk_cache() is > > accessed, > > > > disk_cache() access is deferred until we need to read or write something > > from > > > > the script cache. Start up a migrator then. > > > > > > Given that NTP is considering going out of experiment I suspect this may not > > > change the sequence a lot? > > > > > > (I remember we were considering doing this in disk_cache though, haven't > > checked > > > in why we didn't do so) > > > > Yes, we considered this before but gave it up because we thought > > DiskCacheReader/Writer could interleave migration tasks with regular > read/write > > operations. But, according to the Part B, disk_cache has a queuing mechanism > and > > this seems to be able to work fine. > > Ah, I remember one more thing! We cannot do the migration before the diskcache > is initialized because the migrator uses the ResponseReader/Writer... hmmm... Random ideas... - A) Stop using AppCacheDiskCache for the migration, and directly access raw DiskCache. - B) Keep using AppCacheDiskCache and make a way to defer regular read/write operations until migration operations are complete (probably done in AppCacheDiskCache?). - C) Give up accomplishing 2 A) seems tough because this would be nearly equal to reinventing AppCacheDiskCache infra. B) seems possible, but probably this causes a kind of layering violation between AppCache and ServiceWorker. C) blocks users that are not visiting sw enabled sites. As kinuko@ mentioned, NTP could cause the migration on all users, so I'd like to take C)... WDYT?
Updated! > > **** To accomplish 1: > > > > Store whether migration has occurred in the sw database as part of the initial > > data. Maybe two additional bools: data_migrated_, old_data_deleted_. In the > > expected case, there are no additional tasks to be posted, queued, or run > > compared to what we have now. > > Or could we do the dir check in the same task as the db init? Tentatively Patchset9-11 corporate this way. Any comments are welcome.
On 2015/06/05 05:45:31, nhiroki wrote: > Updated! > > > > **** To accomplish 1: > > > > > > Store whether migration has occurred in the sw database as part of the > initial > > > data. Maybe two additional bools: data_migrated_, old_data_deleted_. In the > > > expected case, there are no additional tasks to be posted, queued, or run > > > compared to what we have now. > > > > Or could we do the dir check in the same task as the db init? > > Tentatively Patchset9-11 corporate this way. Any comments are welcome. s/corporate/incorporate/
> Checking in the same task would be feasible. Additional fields in the database
> could become debt... so I'd prefer to choose this way if possible.
That works too. My concern is mostly about the additional task_and_reply step.
During startup the time a task sits in a Q waiting to be run can be hundreds of
millis, so bundling the check in the existing initTask sgtm.
> Ah, I remember one more thing! We cannot do the migration before the diskcache
> is initialized because the migrator uses the ResponseReader/Writer... hmmm...
I don't understand why that's a problem?
The migrator uses two ServiceWorkerDiskCache instances, one for src and another
for dest, both are initialize, it copies values from one to the other.
Meanwhile, the ServiceWorkerStorage object has a third instance in its
disk_cache_ data member, initiatization of this instance is deferred until the
migrator is done.
Maybe i'm missing something?
> As kinuko@ mentioned, NTP could cause the migration on all users, so I'd like
to
> take C)... WDYT?
Not everyone uses that as the homepage, session restore may not bring you there
upon startup, launching the browser with a url on the cmd line doesn't go there.
******************************
ServiceWorkerDiskCache* ServiceWorkerStorage::disk_cache() {
if (disk_cache_)
return disk_cache_.get();
disk_cache_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend();
base::FilePath path = GetDiskCachePath();
if (path.empty()) {
int rv = disk_cache_->InitWithMemBackend(kMaxMemDiskCacheSize,
net::CompletionCallback());
DCHECK_EQ(net::OK, rv);
return disk_cache_.get();
}
if (needs_migration_) {
disk_cache_->set_waiting_to_initialize(true);
StartMigration(OnMigrationComplete()); // migrator creates and inits its
own instances for src and dst.
return disk_cache_.get();
}
int rv = disk_cache_->InitWithDiskBackend(
path,
kMaxDiskCacheSize,
false,
disk_cache_thread_,
base::Bind(&ServiceWorkerStorage::OnDiskCacheInitialized,
weak_factory_.GetWeakPtr()));
if (rv != net::ERR_IO_PENDING)
OnDiskCacheInitialized(rv);
return disk_cache_.get();
}
void ServiceWorkerStorage::OnMigrationComplete(int rv) {
if (rv != net::OK) {
ScheduleDeleteAndStartOver();
return;
}
// The migrators DiskCache instances should be dead and gone by now.
disk_cache_->set_waiting_to_initialize(false);
int rv = disk_cache_->InitWithDiskBackend(
path,
kMaxDiskCacheSize,
false,
disk_cache_thread_,
base::Bind(&ServiceWorkerStorage::OnDiskCacheInitialized,
weak_factory_.GetWeakPtr()));
if (rv != net::ERR_IO_PENDING)
OnDiskCacheInitialized(rv);
}
Thank you for your comments! Replies only. On 2015/06/05 19:35:41, michaeln wrote: > > Checking in the same task would be feasible. Additional fields in the database > > could become debt... so I'd prefer to choose this way if possible. > > That works too. My concern is mostly about the additional task_and_reply step. > During startup the time a task sits in a Q waiting to be run can be hundreds of > millis, so bundling the check in the existing initTask sgtm. Ok, thanks! > > Ah, I remember one more thing! We cannot do the migration before the diskcache > > is initialized because the migrator uses the ResponseReader/Writer... hmmm... > > I don't understand why that's a problem? > > The migrator uses two ServiceWorkerDiskCache instances, one for src and another > for dest, both are initialize, it copies values from one to the other. > Meanwhile, the ServiceWorkerStorage object has a third instance in its > disk_cache_ data member, initiatization of this instance is deferred until the > migrator is done. > > Maybe i'm missing something? I see! I was thinking the migrator shares the storage's |disk_cache_| instance and their operations are mixed. The third instance should clear it. > > As kinuko@ mentioned, NTP could cause the migration on all users, so I'd like > to > > take C)... WDYT? > > Not everyone uses that as the homepage, session restore may not bring you there > upon startup, launching the browser with a url on the cmd line doesn't go there. You're right. I'll take your original approach :)
Patchset #12 (id:470001) has been deleted
Updated! The patchset 13 should accomplish the goal (2). Can you take another look? Thanks!
Some quick comments. BTW I've started using "git-cl lint" on my patches, you might want to try too. https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.h (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.h:4: no include guard? https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:54: ASSERT_FALSE(registration); It's a bit unexpected to assert these in a function called OnRegistrationFound. https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1002: needs_disk_cache_migration_ = true; I am confused. Why is the |needs_disk_cache_migration| parameter not used? https://codereview.chromium.org/1152543002/diff/510001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1152543002/diff/510001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:36838: + units="millisecond"> nit: milliseconds
Thank you! Updated. https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.h (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.h:4: On 2015/06/08 09:05:54, falken wrote: > no include guard? Done. https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:54: ASSERT_FALSE(registration); On 2015/06/08 09:05:54, falken wrote: > It's a bit unexpected to assert these in a function called OnRegistrationFound. These asserts were used when I tested the patchset. I think these are no longer necessary, so removed. https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/510001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1002: needs_disk_cache_migration_ = true; On 2015/06/08 09:05:54, falken wrote: > I am confused. Why is the |needs_disk_cache_migration| parameter not used? Good catch!! The parameter must be used. Fixed. https://codereview.chromium.org/1152543002/diff/510001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1152543002/diff/510001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:36838: + units="millisecond"> On 2015/06/08 09:05:54, falken wrote: > nit: milliseconds Fixed (hmmm... I copied this from other SW UMAs... I'll fix them in a separate CL)
This is looking pretty good. I've got some questions when it comes to migration error and retry cases. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1002: needs_disk_cache_migration_ = needs_disk_cache_migration; Curiosity: I wish we didn't have to migrate at all because too many thing can go wrong. What is it that is forced us to migrate instead of going with the comparatively simple starting with a clean slate? https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1436: // Initialize the dest DiskCache. Since a migration attempt can be started multiple times, should the first step be to delete any partial results from an earlier attempt? https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1473: migrator_ptr->Start(base::Bind( The migrator will continue to run even after ServiceWorkerStorage has been deleted, yet the results of a migrator left running detached from its storage will never be used. Seems like we should abort the migrator in this case to avoid it doing a bunch of unwanted io during shutdown. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1476: base::Passed(dest_disk_cache.Pass()))); having ownership of src and dst bound into the closure works, but having the migrator own them outright seems easier to grok https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1506: AbortDiskCacheMigration( Seems unfortunate to let this error prevent migration completion since the dest cache is successfully written and ready to go? Likely the call in line 1523 will also fail to delete the old. Hmmm, if we're unable to delete the old dir we'll be wedged forever trying to migrate? https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1515: void ServiceWorkerStorage::AbortDiskCacheMigration( This method doesn't actually abort anything, it handles a migration failure. Maybe rename the method? https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1697: needs_disk_cache_migration = base::DirectoryExists(old_disk_cache_path); I'm wondering about pathological cases where migration fails? How do we handle different failure modes. As coded looks like we could repeatedly try to migrate during a browsing session. I'm back to wondering if we should store whether migration has happened or is needed in the db proper to handle unable to delete the old case? Also wondering if the name of the scriptcache directory should be part of the db schema to handle being unable to delete partial results from an earlier migration attempt? https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:387: void DidInitializeDiskCacheForMigration(bool* is_failed, Since the storage class is pretty big, it might be nice to hoist the code to initialize the src and dest caches into the migrator class.
Patchset #17 (id:590001) has been deleted
Patchset #17 (id:610001) has been deleted
Patchset #17 (id:630001) has been deleted
Patchset #18 (id:670001) has been deleted
Patchset #17 (id:650001) has been deleted
Patchset #17 (id:690001) has been deleted
Thank you for carefully reviewing! Updated the patchset. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1002: needs_disk_cache_migration_ = needs_disk_cache_migration; On 2015/06/08 21:36:33, michaeln wrote: > Curiosity: I wish we didn't have to migrate at all because too many thing can go > wrong. What is it that is forced us to migrate instead of going with the > comparatively simple starting with a clean slate? Yeah... it's definitely simpler and easier to achieve..., but it'd be preferable to avoid losing (push) registrations: https://code.google.com/p/chromium/issues/detail?id=487482#c13 https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1436: // Initialize the dest DiskCache. On 2015/06/08 21:36:33, michaeln wrote: > Since a migration attempt can be started multiple times, should the first step > be to delete any partial results from an earlier attempt? Done. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1473: migrator_ptr->Start(base::Bind( On 2015/06/08 21:36:33, michaeln wrote: > The migrator will continue to run even after ServiceWorkerStorage has been > deleted, yet the results of a migrator left running detached from its storage > will never be used. Seems like we should abort the migrator in this case to > avoid it doing a bunch of unwanted io during shutdown. I changed the storage to own the migrator. When the storage is destroyed, the migrator is also destroyed and running/pending migration tasks bound with weakptr<migrator> are aborted. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1476: base::Passed(dest_disk_cache.Pass()))); On 2015/06/08 21:36:33, michaeln wrote: > having ownership of src and dst bound into the closure works, but having the > migrator own them outright seems easier to grok Moved the initialization code from storage to migrator. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1506: AbortDiskCacheMigration( On 2015/06/08 21:36:33, michaeln wrote: > Seems unfortunate to let this error prevent migration completion since the dest > cache is successfully written and ready to go? Likely the call in line 1523 will > also fail to delete the old. Hmmm, if we're unable to delete the old dir we'll > be wedged forever trying to migrate? Changed this sequence. The latest patchset records the old diskcache directory in the database (INITDATA_PURGEABLE_FILE) and tries to delete it independently from the migration attempt. https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1515: void ServiceWorkerStorage::AbortDiskCacheMigration( On 2015/06/08 21:36:33, michaeln wrote: > This method doesn't actually abort anything, it handles a migration failure. > Maybe rename the method? (This function is gone) https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1697: needs_disk_cache_migration = base::DirectoryExists(old_disk_cache_path); On 2015/06/08 21:36:33, michaeln wrote: > I'm wondering about pathological cases where migration fails? How do we handle > different failure modes. As coded looks like we could repeatedly try to migrate > during a browsing session. I'm back to wondering if we should store whether > migration has happened or is needed in the db proper to handle unable to delete > the old case? Added INITDATA_DISKCACHE_VERSION field in the database. We should be able to distinguish the migration state by the version number. > Also wondering if the name of the scriptcache directory should be > part of the db schema to handle being unable to delete partial results from an > earlier migration attempt? In the latest CL, if the migrator is still running on the dtor of the storage, the new diskcache directory which contains partial results is recorded as a purgeable file in the database. Purgeable files are removed on the next system startup. (We might not have to do this if we delete partial results immediately before the migration as you commented at line 1436?) https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... File content/browser/service_worker/service_worker_storage.h (right): https://codereview.chromium.org/1152543002/diff/550001/content/browser/servic... content/browser/service_worker/service_worker_storage.h:387: void DidInitializeDiskCacheForMigration(bool* is_failed, On 2015/06/08 21:36:33, michaeln wrote: > Since the storage class is pretty big, it might be nice to hoist the code to > initialize the src and dest caches into the migrator class. Moved the initialization code from storage to migrator.
sorry for the delay https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:76: // key: "INITDATA_DISKCACHE_VERSION" It feels a little early to generalize a diskcache versioning scheme file deletion mechanism. I think the plan is to remove the migration code after a few release cycles. Would you expect to also remove this code at this time? Consider something more specific to this blockfile -> simplecache migration, and then to remove the code at some point. https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:426: *disk_cache_version = 0; should this be kCurrentDiskCacheVersion? https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1028: if (filename.empty()) when would we ever expect empty names here? https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.cc:280: src_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend(); android? https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (left): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1374: disk_cache_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend(); Ooops, does this cause the blockfile backend to be used on android too? I think we may have done that unintentionally in https://codereview.chromium.org/1155063002 https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:65: // Simple backend (http://crbug.com/487482). How are we handling android? It already uses the simple backend. Should the migrator degenerate into a rename() function call in that case? https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1040: for (const std::string& filename : purgeable_files) { Can you defer the start of purging with BrowserThread::PostAfterStartupTask to reduce IO during startup. https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1044: service_worker_directory.AppendASCII(filename), true), Having a general'ish purpose file deletion mechanism spooks me. What if filename = "../../../Documents"? Should we care about that? https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1492: // Update the disk cache version. I'm wondering about the order of the operations in here? What if chrome crashes in between any steps taken here. The steps taken are not in a deterministic order since tasks are posted to different threads concurrently which makes it trickier to reason about. I think as coded, we could delete the old dir prior to having updated the version number in the db, leaving things in an inconsistent state. * insert old dir into db's purgeable list * delete old dir * delete old dir from db's purgeable list * update db with new version number I think we might want to alter the ordering and grouping of operations. First record our work in the main database beginTransaction update db with new version number insert old dir into db's purgeable list end Wait for xaction completion, then InitializeDiskCache() so no changes to the diskcache can be made until we've committed the db updates. Finally, after browser startup, delete the old directory
Thank you for reviewing!! Hmmm... I've kept this as one CL in order to easily merge, but it's getting bigger and harder to keep than I expected... (and also probably harder to review for you?), so I tried to factor out migrator changes including Android support into a separate CL: https://codereview.chromium.org/1181573002/ Please let me know if you prefer to review all in this CL. Thanks.
Updated. I hope this version works well...! https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:76: // key: "INITDATA_DISKCACHE_VERSION" On 2015/06/11 00:52:38, michaeln wrote: > It feels a little early to generalize a diskcache versioning scheme file > deletion mechanism. I think the plan is to remove the migration code after a few > release cycles. Would you expect to also remove this code at this time? I was thinking to keep these fields after migration code are removed and abuse them for something. But, yeah..., it looks like overkill. > Consider something more specific to this blockfile -> simplecache migration, and > then to remove the code at some point. Ok, let's simplify them (Done). https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:426: *disk_cache_version = 0; On 2015/06/11 00:52:38, michaeln wrote: > should this be kCurrentDiskCacheVersion? (This code is gone) https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1028: if (filename.empty()) On 2015/06/11 00:52:38, michaeln wrote: > when would we ever expect empty names here? (This code is gone) https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.cc:280: src_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend(); On 2015/06/11 00:52:38, michaeln wrote: > android? Done in the separate CL: https://codereview.chromium.org/1181573002/ https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (left): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1374: disk_cache_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend(); On 2015/06/11 00:52:38, michaeln wrote: > Ooops, does this cause the blockfile backend to be used on android too? I think > we may have done that unintentionally in > https://codereview.chromium.org/1155063002 Fixed in the separate CL: https://codereview.chromium.org/1181573002/ https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:65: // Simple backend (http://crbug.com/487482). On 2015/06/11 00:52:38, michaeln wrote: > How are we handling android? It already uses the simple backend. Should the > migrator degenerate into a rename() function call in that case? Done in the separate CL: https://codereview.chromium.org/1181573002/ (The migrator just calls base::Move() in the case) https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1040: for (const std::string& filename : purgeable_files) { On 2015/06/11 00:52:38, michaeln wrote: > Can you defer the start of purging with BrowserThread::PostAfterStartupTask to > reduce IO during startup. Done. Purging is deferred until an initial diskcache access. https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1044: service_worker_directory.AppendASCII(filename), true), On 2015/06/11 00:52:38, michaeln wrote: > Having a general'ish purpose file deletion mechanism spooks me. > > What if filename = "../../../Documents"? Should we care about that? (This code is gone) https://codereview.chromium.org/1152543002/diff/750001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1492: // Update the disk cache version. On 2015/06/11 00:52:38, michaeln wrote: > I'm wondering about the order of the operations in here? What if chrome crashes > in between any steps taken here. The steps taken are not in a deterministic > order since tasks are posted to different threads concurrently which makes it > trickier to reason about. I think as coded, we could delete the old dir prior to > having updated the version number in the db, leaving things in an inconsistent > state. > > * insert old dir into db's purgeable list > * delete old dir > * delete old dir from db's purgeable list > * update db with new version number > > > I think we might want to alter the ordering and grouping of operations. > > First record our work in the main database > beginTransaction > update db with new version number > insert old dir into db's purgeable list > end > > Wait for xaction completion, then InitializeDiskCache() so no changes to the > diskcache can be made until we've committed the db updates. > > Finally, after browser startup, delete the old directory Revised these operations. The new version... (1) introduces "migration state" instead of "version" and "purgeable list". (2) groups operations to update a migration state to NEEDS_TO_DELETE, to delete an old dir, and to update the state to MIGRATED in this order (see DeleteOldDiskCacheInDB()). * The old dir should be deletable on the database thread because any diskcache doesn't touch it any more. * If browser crashes before NEEDS_TO_DELETE is recorded, the migrator retries from the beginning. * If browser crashes after NEEDS_TO_DELETE is recorded. the migrator does not re-run and this grouped operations are executed concurrently with normal diskcache operations. (3) defers the start of diskcache initialization until the migration state is updated to NEEDS_TO_DELETE or MIGRATED.
This is shaping up pretty good. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.h:106: DISKCACHE_MIGRATED, Can MIGRATED and NOT_USED be merged into MIGRATION_NOT_NEEDED? https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1447: case ServiceWorkerDatabase::DISKCACHE_NOT_USED: { Hmmm, maybe this state doesn't need to be here? Looks like we do IO and thread hopping to transition to the MIGRATED state. What if instead of returning NOT_USED to begin with we return MIGRATED or more accurately MIGRATION_NOT_NEEDED? https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1479: database_task_manager_->GetTaskRunner()->PostTask( Is there a reason to not schedule this task va BrowserThread::PostAfterStartupTask? At this point, we could be serving up the SW for the NTP long before startup is complete. Seems like we could defer this cleanup task easily enough... unless there's reason not to? BrowserThread::PostAfterStartupTask( FROM_HERE, database_task_manager_->GetTaskRunner(), base::Bind(...));
Thanks! I have a question about the NOT_NEEDED case (see my comment) https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1447: case ServiceWorkerDatabase::DISKCACHE_NOT_USED: { On 2015/06/12 01:55:45, michaeln wrote: > Hmmm, maybe this state doesn't need to be here? Looks like we do IO and thread > hopping to transition to the MIGRATED state. What if instead of returning > NOT_USED to begin with we return MIGRATED or more accurately > MIGRATION_NOT_NEEDED? In this case, how can we avoid an unnecessary migration on the next browser startup? Such a migration could delete the dest diskcache. Should we write the migration state in the database at ReadDiskCacheMigrationState? Or the migrator just skips if the src directory is empty and updates the state at the end of the migration? https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1479: database_task_manager_->GetTaskRunner()->PostTask( On 2015/06/12 01:55:45, michaeln wrote: > Is there a reason to not schedule this task va > BrowserThread::PostAfterStartupTask? At this point, we could be serving up the > SW for the NTP long before startup is complete. Seems like we could defer this > cleanup task easily enough... unless there's reason not to? > > BrowserThread::PostAfterStartupTask( > FROM_HERE, > database_task_manager_->GetTaskRunner(), > base::Bind(...)); > I didn't know this helper. There should be no obstacle to use it here. Replaced it.
https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1447: case ServiceWorkerDatabase::DISKCACHE_NOT_USED: { On 2015/06/12 03:12:14, nhiroki wrote: > On 2015/06/12 01:55:45, michaeln wrote: > > Hmmm, maybe this state doesn't need to be here? Looks like we do IO and thread > > hopping to transition to the MIGRATED state. What if instead of returning > > NOT_USED to begin with we return MIGRATED or more accurately > > MIGRATION_NOT_NEEDED? > > In this case, how can we avoid an unnecessary migration on the next browser > startup? Such a migration could delete the dest diskcache. Should we write the > migration state in the database at ReadDiskCacheMigrationState? This can avoid an extra thread hopping, but needs a database write in the startup sequence (in ReadInitialDataFromDB). > Or the migrator just skips if the src directory is empty and updates the state at the end of the migration? In this way, the next startup goes into NEEDS_TO_MIGRATE state and defers diskcache initialization, too.
i was having a hard time reasoning about it, so i tried to dumb it down so i could understand it, wdyt? https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:5: #include "content/browser/service_worker/service_worker_database.h" In current sources, when chrome is run for the first time, storage initialization doesn't create a database on disk. I'd think we can keep it that way. When run for the first time, here really is nothing to migrate. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:95: const char kDiskCacheMigratedKey[] = "INITDATA_DISKCACHE_MIGRATED"; kDiskCacheMigrationNotNeededKey = "INITDATA_DISKCACHE_MIGRATION_NOT_NEEDED"; https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:96: const char kOldDiskCacheExistsKey[] = "INITDATA_OLD_DISKCACHE_EXISTS"; INITDATA_OLD_DISKCACHE_DELETION_NOT_NEEDED https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:435: ServiceWorkerDatabase::ReadDiskCacheMigrationState( The simple ReadX and WriteX names are misleading. These functions do more than blindly read/write the provided value to the db. There are two different values in the db that map to 4 different states. I'm finding it difficult to reason about and wonder if a dumbed down db layer would help? What if... db->IsDiskCacheMigrationNeeded() return true or false db->IsOldDiskCacheDeletionNeeded() returns true or false I think these two bits are what the higher level storage class really needs to know. Lets say for a non-existent or brand new database, these values default to. false for migrationNeeded false for oldDeletetionNeeded For an existing database with no record of these inside (no keys present) true for migrationNeeded true for oldDeletionNeeded When migration is done dbresult = db->SetDiskCacheMigrationNotNeeded() puts kMigrationNotNeeded key and when old directory deletion is done dbresult = db->SetOldDiskCacheDeletionNotNeeded() puts kDiskCacheDeletionNotNeeded key The Is getters use LazyOpen(false) and the Setters use LazyOpen(true). I like having all the app logic smarts in the Storage class and the DB class's job is to safely put and get values that need to be persisted. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1502: state_ = INITIALIZED; we're initializing a brand new db, put the default values just like we put the db version number in batch->Put(kDiskCacheMigrationNotNeededKey); batch->Put(kOldDiskCacheDeletionNotNeededKey); https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:934: ServiceWorkerDatabase::DISKCACHE_UNKNOWN_STATE), is_migration_needed_(false) // we don't know yet is_old_deletion_needed_(false) https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1447: case ServiceWorkerDatabase::DISKCACHE_NOT_USED: { On 2015/06/12 03:32:33, nhiroki wrote: > On 2015/06/12 03:12:14, nhiroki wrote: > > On 2015/06/12 01:55:45, michaeln wrote: > > > Hmmm, maybe this state doesn't need to be here? Looks like we do IO and > thread > > > hopping to transition to the MIGRATED state. What if instead of returning > > > NOT_USED to begin with we return MIGRATED or more accurately > > > MIGRATION_NOT_NEEDED? > > > > In this case, how can we avoid an unnecessary migration on the next browser > > startup? Such a migration could delete the dest diskcache. Should we write the > > migration state in the database at ReadDiskCacheMigrationState? > > This can avoid an extra thread hopping, but needs a database write in the > startup sequence (in ReadInitialDataFromDB). > > > Or the migrator just skips if the src directory is empty and updates the state > at the end of the migration? > > In this way, the next startup goes into NEEDS_TO_MIGRATE state and defers > diskcache initialization, too. my head hurts :) https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1446: switch (disk_cache_migration_state_) { if (migration_needed_) start it; // when its done, postafterstartuptask to delete old else if (deletion_needed_) postafterstartuptask to do it https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1733: if (!base::DeleteFile(disk_cache_path, true)) { it would be good to defer this part till 'afterstartup'
Patchset #25 (id:870001) has been deleted
Thank you! Updated. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:5: #include "content/browser/service_worker/service_worker_database.h" On 2015/06/12 20:47:57, michaeln wrote: > In current sources, when chrome is run for the first time, storage > initialization doesn't create a database on disk. I'd think we can keep it that > way. When run for the first time, here really is nothing to migrate. Acknowledged. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:95: const char kDiskCacheMigratedKey[] = "INITDATA_DISKCACHE_MIGRATED"; On 2015/06/12 20:47:58, michaeln wrote: > kDiskCacheMigrationNotNeededKey = "INITDATA_DISKCACHE_MIGRATION_NOT_NEEDED"; Done. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:96: const char kOldDiskCacheExistsKey[] = "INITDATA_OLD_DISKCACHE_EXISTS"; On 2015/06/12 20:47:57, michaeln wrote: > INITDATA_OLD_DISKCACHE_DELETION_NOT_NEEDED Done. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:435: ServiceWorkerDatabase::ReadDiskCacheMigrationState( On 2015/06/12 20:47:58, michaeln wrote: > The simple ReadX and WriteX names are misleading. These functions do more than > blindly read/write the provided value to the db. There are two different values > in the db that map to 4 different states. I'm finding it difficult to reason > about and wonder if a dumbed down db layer would help? > > What if... > > db->IsDiskCacheMigrationNeeded() return true or false > db->IsOldDiskCacheDeletionNeeded() returns true or false > > I think these two bits are what the higher level storage class really needs to > know. > > Lets say for a non-existent or brand new database, these values default to. > > false for migrationNeeded > false for oldDeletetionNeeded > > For an existing database with no record of these inside (no keys present) > > true for migrationNeeded > true for oldDeletionNeeded > > When migration is done > > dbresult = db->SetDiskCacheMigrationNotNeeded() > puts kMigrationNotNeeded key > > and when old directory deletion is done > > dbresult = db->SetOldDiskCacheDeletionNotNeeded() > puts kDiskCacheDeletionNotNeeded key > > > The Is getters use LazyOpen(false) and the Setters use LazyOpen(true). > > I like having all the app logic smarts in the Storage class and the DB class's > job is to safely put and get values that need to be persisted. > Thank you for the detailed explanation. This looks much simpler! I updated the CL in this way. https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1502: state_ = INITIALIZED; On 2015/06/12 20:47:58, michaeln wrote: > we're initializing a brand new db, put the default values just like we put the > db version number in > > batch->Put(kDiskCacheMigrationNotNeededKey); > batch->Put(kOldDiskCacheDeletionNotNeededKey); Ah..., yes. This is the most suitable place to set these default values (and I should have noticed... hmm) https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/790001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:934: ServiceWorkerDatabase::DISKCACHE_UNKNOWN_STATE), On 2015/06/12 20:47:58, michaeln wrote: > is_migration_needed_(false) // we don't know yet > is_old_deletion_needed_(false) Done. https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1446: switch (disk_cache_migration_state_) { On 2015/06/12 20:47:58, michaeln wrote: > if (migration_needed_) > start it; // when its done, postafterstartuptask to delete old > else if (deletion_needed_) > postafterstartuptask to do it > Done. https://codereview.chromium.org/1152543002/diff/830001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1733: if (!base::DeleteFile(disk_cache_path, true)) { On 2015/06/12 20:47:58, michaeln wrote: > it would be good to defer this part till 'afterstartup' Done.
lgtm, maybe ping matt or kinuko want to take another look too https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_database.h:380: // kOldDiskCacheDeletionNotNeededKey when initializing a new database. #if defined(UNIT_TEST) and the comment might not be needed https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1449: } else if (old_disk_cache_deletion_needed_) { since you have an early return above, the else is not needed https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1473: // Lazily delete the old diskcache. maybe comment there's no reason to update the database in this case https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1506: // Lazily delete the old diskcache. maybe comment and update the database in this case
Thank you for patiently reviewing! https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_database.h:380: // kOldDiskCacheDeletionNotNeededKey when initializing a new database. On 2015/06/15 23:03:24, michaeln wrote: > #if defined(UNIT_TEST) and the comment might not be needed Done. https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1449: } else if (old_disk_cache_deletion_needed_) { On 2015/06/15 23:03:24, michaeln wrote: > since you have an early return above, the else is not needed Done. https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1473: // Lazily delete the old diskcache. On 2015/06/15 23:03:24, michaeln wrote: > maybe comment there's no reason to update the database in this case Done. https://codereview.chromium.org/1152543002/diff/890001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1506: // Lazily delete the old diskcache. On 2015/06/15 23:03:24, michaeln wrote: > maybe comment and update the database in this case Done.
falken@, kinuko@: This CL was drastically changed after you reviewed. Can you take a second look? Thanks!
nhiroki@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ for histograms.xml owner
lgtm https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_database.h:380: void set_skip_writing_diskcache_migration_state_on_init() { nit: can you add "_for_testing" to the function and variable names? https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:357: // Simulatie an existing database. Simulate https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:498: EXPECT_TRUE(base::DirectoryExists(storage()->GetOldDiskCachePath())); Question: Is the old disk cache directory never deleted in this case? I guess it's not a big deal if so. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1472: // Give up a migration and recreate the whole storage. nit: reads better without the "a" ("a" sounds like it's one migration of many) https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1498: // Give up a migration and recreate the whole storage. nit: here too It looks like you could just copy+paste lines 1469-1480 and 1495-1506 into a function OnMigrationFailed()... would it make sense to do so?
Thank you! Updated. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_database.h:380: void set_skip_writing_diskcache_migration_state_on_init() { On 2015/06/16 04:28:10, falken wrote: > nit: can you add "_for_testing" to the function and variable names? Done. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:357: // Simulatie an existing database. On 2015/06/16 04:28:10, falken wrote: > Simulate Done. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:498: EXPECT_TRUE(base::DirectoryExists(storage()->GetOldDiskCachePath())); On 2015/06/16 04:28:10, falken wrote: > Question: Is the old disk cache directory never deleted in this case? I guess > it's not a big deal if so. Yes, never deleted. I think we don't have to handle this case because this is an intentional case for testing and basically the old directory should not exist when the dasabase is empty. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_storage.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1472: // Give up a migration and recreate the whole storage. On 2015/06/16 04:28:10, falken wrote: > nit: reads better without the "a" ("a" sounds like it's one migration of many) Done. https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_storage.cc:1498: // Give up a migration and recreate the whole storage. On 2015/06/16 04:28:10, falken wrote: > nit: here too > > It looks like you could just copy+paste lines 1469-1480 and 1495-1506 into a > function OnMigrationFailed()... would it make sense to do so? Done. Factored them out into OnMigrationFailed.
https://codereview.chromium.org/1152543002/diff/930001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.cc (right): https://codereview.chromium.org/1152543002/diff/930001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.cc:54: migrated_resources, 1, 1000, 50); This looks like an UMA_HISTOGRAM_COUNTS_1000, which doesn't exist - but apparently lots of people already use these params: https://code.google.com/p/chromium/codesearch#search/&q=%221,%201000,%2050)%2... Can you add UMA_HISTOGRAM_COUNTS_1000() to histogram_macros.h?
Thanks! Updated. https://codereview.chromium.org/1152543002/diff/930001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator.cc (right): https://codereview.chromium.org/1152543002/diff/930001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator.cc:54: migrated_resources, 1, 1000, 50); On 2015/06/16 15:31:15, Alexei Svitkine wrote: > This looks like an UMA_HISTOGRAM_COUNTS_1000, which doesn't exist - but > apparently lots of people already use these params: > > https://code.google.com/p/chromium/codesearch#search/&q=%221,%201000,%2050)%2... > > Can you add UMA_HISTOGRAM_COUNTS_1000() to histogram_macros.h? Done.
lgtm
https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:498: EXPECT_TRUE(base::DirectoryExists(storage()->GetOldDiskCachePath())); On 2015/06/16 07:12:16, nhiroki wrote: > On 2015/06/16 04:28:10, falken wrote: > > Question: Is the old disk cache directory never deleted in this case? I guess > > it's not a big deal if so. > > Yes, never deleted. I think we don't have to handle this case because this is an > intentional case for testing and basically the old directory should not exist > when the dasabase is empty. If we're worried about being sure to clean up, maybe.. * when there is no existing db, needs_deletion = false (so we don't repeatedly try to delete on each startup) * but when we actually create a db for the first and write the initial values, make it look like deletion is needed (so we delete on the next startup and record that we did so to not repeatedly try to delete) Would that work?
https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... File content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc (right): https://codereview.chromium.org/1152543002/diff/910001/content/browser/servic... content/browser/service_worker/service_worker_disk_cache_migrator_unittest.cc:498: EXPECT_TRUE(base::DirectoryExists(storage()->GetOldDiskCachePath())); On 2015/06/16 20:50:24, michaeln wrote: > On 2015/06/16 07:12:16, nhiroki wrote: > > On 2015/06/16 04:28:10, falken wrote: > > > Question: Is the old disk cache directory never deleted in this case? I > guess > > > it's not a big deal if so. > > > > Yes, never deleted. I think we don't have to handle this case because this is > an > > intentional case for testing and basically the old directory should not exist > > when the dasabase is empty. > > If we're worried about being sure to clean up, maybe.. > > * when there is no existing db, needs_deletion = false (so we don't repeatedly > try to delete on each startup) > > * but when we actually create a db for the first and write the initial values, > make it look like deletion is needed (so we delete on the next startup and > record that we did so to not repeatedly try to delete) > > Would that work? SGTM. Updated the database initialization and this test.
https://codereview.chromium.org/1152543002/diff/970001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/970001/content/browser/servic... content/browser/service_worker/service_worker_database.h:379: #if defined(UNIT_TEST) Hmmm... only win_chromium_compile_dbg_ng fails to link this due to "unresolved external symbol", win_chromium_rel_ng succeeds though. We should unwrap this...?
https://codereview.chromium.org/1152543002/diff/970001/content/browser/servic... File content/browser/service_worker/service_worker_database.h (right): https://codereview.chromium.org/1152543002/diff/970001/content/browser/servic... content/browser/service_worker/service_worker_database.h:379: #if defined(UNIT_TEST) On 2015/06/17 02:17:55, nhiroki wrote: > Hmmm... only win_chromium_compile_dbg_ng fails to link this due to "unresolved > external symbol", win_chromium_rel_ng succeeds though. We should unwrap this...? This setter needs to be inlined, but the dbg-bot does not do so and then a linker fails to link. Probably we should also specify !defined(NDEBUG) for the case like: https://code.google.com/p/chromium/codesearch#chromium/src/components/visited... And see the patchset 30. (yhirano@ kindly helped me investigate, thank you!) BTW, the presubmit script checks "_for_testing" suffix and shows a warning message if it's used in non-test files, so we might not have to wrap this with defined(UNIT_TEST) for such a check. And this could be optimized out in release build because there is no caller.
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, falken@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1152543002/#ps1010001 (title: "remove defined(UNIT_TEST)")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152543002/1010001
On 2015/06/17 05:41:06, nhiroki wrote: > (yhirano@ kindly helped me investigate, thank you!) +yhirano@ to CC
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaeln@chromium.org, asvitkine@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/1152543002/#ps1030001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152543002/1030001
The CQ bit was unchecked by nhiroki@chromium.org
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152543002/1030001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152543002/1030001
Message was sent while issue was closed.
Committed patchset #32 (id:1030001)
Message was sent while issue was closed.
Patchset 32 (id:??) landed as https://crrev.com/d1c9f9fbe26992894355b68f0445e03c2061908f Cr-Commit-Position: refs/heads/master@{#334811} |
