[appcache] Don't write during startup.

[AppCache] Avoid frequent small writes and writing during startup by batching together updates to last access times.

BUG=460265
Committed: https://crrev.com/56820bdb2a9bcddca9528b16e79d7339f218336f
Cr-Commit-Position: refs/heads/master@{#317418}

[michaeln, 2015-02-13 21:04:03]

michaeln@chromium.org changed reviewers:
+ cmumford@chromium.org

[michaeln, 2015-02-13 21:04:04]

not quite ready yet, but here's a preview

[cmumford, 2015-02-13 23:06:15]

https://codereview.chromium.org/924723002/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/924723002/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_database.cc:432: if (!transaction.Commit())
If we fail to update or commit the last access times then does it make sense to
still keep this map around? won't it likely just fail again?

[michaeln, 2015-02-14 01:35:03]

https://codereview.chromium.org/924723002/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/924723002/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_database.cc:432: if (!transaction.Commit())
On 2015/02/13 23:06:15, cmumford wrote:
> If we fail to update or commit the last access times then does it make sense
to
> still keep this map around? won't it likely just fail again?

Done, the method easier to read now too.

[cmumford, 2015-02-17 19:26:39]

What do you think about splitting these into two separate CL's? I guess it's
small enough, but the two changes are unrelated.

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
content/browser/appcache/appcache_storage_impl.cc:316: const base::TimeDelta
k5Minutes = base::TimeDelta::FromMinutes(5);
My preference would be to have a name that describes it's purpose, and not it's
value. kDelay is preferable to my eye.

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
content/browser/appcache/appcache_storage_impl.cc:1938: }
Space

[michaeln, 2015-02-17 19:46:26]

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
content/browser/appcache/appcache_storage_impl.cc:316: const base::TimeDelta
k5Minutes = base::TimeDelta::FromMinutes(5);
On 2015/02/17 19:26:39, cmumford wrote:
> My preference would be to have a name that describes it's purpose, and not
it's
> value. kDelay is preferable to my eye.

Done.

https://codereview.chromium.org/924723002/diff/40001/content/browser/appcache...
content/browser/appcache/appcache_storage_impl.cc:1938: }
On 2015/02/17 19:26:39, cmumford wrote:
> Space

Done.

[michaeln, 2015-02-18 23:43:07]

On 2015/02/17 19:26:39, cmumford wrote:
> What do you think about splitting these into two separate CL's? I guess it's
> small enough, but the two changes are unrelated.

Ok, I removed the tracking that is unrelated to the lazy committing but left
those that are (those that measure the tasks affected by the change and tasks
involved during startup).

[michaeln, 2015-02-19 22:49:50]

michaeln@chromium.org changed reviewers:
+ dmurph@chromium.org

[michaeln, 2015-02-19 22:50:26]

dan, ptal?

[dmurph, 2015-02-20 00:04:45]

lgtm with nits

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_database.h (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_database.h:121: bool LazyUpdateLastAccessTime(
Can you comment that calling 'commit' is required for the times to be saved?

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl.cc:1915: void
AppCacheStorageImpl::OnCommitLastAccessTimes() {
This sounds like 'commit last action times' already occurred. perhaps rename to
just CommitLastAccessTimes? Or what about CommitLastAccessTimesFromDelay?

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl.h (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl.h:121: void
LazilyCommitLastAccessTimes();
Can you put a comment saying the magnitude of time it will take? And that it's
batching calling in that time period?

[cmumford, 2015-02-20 17:25:47]

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_database.h (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_database.h:121: bool LazyUpdateLastAccessTime(
On 2015/02/20 00:04:45, dmurph wrote:
> Can you comment that calling 'commit' is required for the times to be saved?

I don't believe it is (explicitly) as the destructor calls
CommitLazyLastAccessTimes.

[cmumford, 2015-02-20 19:22:21]

lgtm

[dmurph, 2015-02-20 19:33:59]

On 2015/02/20 at 17:25:47, cmumford wrote:
>
https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
> File content/browser/appcache/appcache_database.h (right):
> 
>
https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
> content/browser/appcache/appcache_database.h:121: bool
LazyUpdateLastAccessTime(
> On 2015/02/20 00:04:45, dmurph wrote:
> > Can you comment that calling 'commit' is required for the times to be saved?
> 
> I don't believe it is (explicitly) as the destructor calls
CommitLazyLastAccessTimes.

Ah, so then can you mention that instead? That the destructor will commit the
lazy ones?

[michaeln, 2015-02-20 20:21:59]

New patchsets have been uploaded after l-g-t-m from
dmurph@chromium.org,cmumford@chromium.org

[michaeln, 2015-02-20 20:24:39]

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_database.h (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_database.h:121: bool LazyUpdateLastAccessTime(
On 2015/02/20 17:25:47, cmumford wrote:
> On 2015/02/20 00:04:45, dmurph wrote:
> > Can you comment that calling 'commit' is required for the times to be saved?
> 
> I don't believe it is (explicitly) as the destructor calls
> CommitLazyLastAccessTimes.

Done.

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
File content/browser/appcache/appcache_storage_impl.cc (right):

https://codereview.chromium.org/924723002/diff/100001/content/browser/appcach...
content/browser/appcache/appcache_storage_impl.cc:1915: void
AppCacheStorageImpl::OnCommitLastAccessTimes() {
On 2015/02/20 00:04:45, dmurph wrote:
> This sounds like 'commit last action times' already occurred. perhaps rename
to
> just CommitLastAccessTimes? Or what about CommitLastAccessTimesFromDelay?

Done.

[michaeln, 2015-02-20 20:24:50]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-02-20 20:25:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/924723002/140001

[commit-bot: I haz the power, 2015-02-20 22:09:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-20 22:09:54]

Patchset 8 (id:??) landed as
https://crrev.com/56820bdb2a9bcddca9528b16e79d7339f218336f
Cr-Commit-Position: refs/heads/master@{#317418}