[CacheStorage] Use backend size to determine change in cache size in put and delete

[CacheStorage] Use backend size to determine change in cache size in put and delete

Rather than just report the size of the metadata and the body, report the difference in cache size from before and after the operation. This fits better with GetOriginSize which sums cache sizes.

BUG=581709
Committed: https://crrev.com/30395012ca331a41f784ce0d129c7eaf4d79f458
Cr-Commit-Position: refs/heads/master@{#375295}

[jkarlin, 2016-02-08 20:58:49]

Description was changed from

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

This is part of a larger effort to use the same measurement system for
cache size across all of cache storage (the backend's size estimate).

BUG=581709
==========

to

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

Rather than just report the size of the metadata and the body, report the
difference in cache size from before and after the operation. This fits better
with GetOriginSize which sums cache sizes.

BUG=581709
==========

[jkarlin, 2016-02-08 20:59:07]

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

[jkarlin, 2016-02-08 20:59:07]

michaeln@ PTAL at everything, thanks!

[michaeln, 2016-02-09 22:23:03]

can multiple ops be running concurrently? i'm wondering if a change made by one
op can be double booked by a second op?

start put - size@start = N
start delete - size@start = N
end delete - size@end = N - D --> deltaReported = -D
end put 0 size@end = (N - D) + P --> deltaReported = P - D

If that's possible, a different approach could be to try to attribute speicif
size changes to specific ops. Instead sample how big it is every now and then,
report any delta from last time.

[jkarlin, 2016-02-10 12:27:03]

On 2016/02/09 22:23:03, michaeln wrote:
> can multiple ops be running concurrently? i'm wondering if a change made by
one
> op can be double booked by a second op?
> 
> start put - size@start = N
> start delete - size@start = N
> end delete - size@end = N - D --> deltaReported = -D
> end put 0 size@end = (N - D) + P --> deltaReported = P - D
> 
> If that's possible, a different approach could be to try to attribute speicif
> size changes to specific ops. Instead sample how big it is every now and then,
> report any delta from last time.

It's not possible, all ops are run sequentially, for now. But it should be
possible one day, and I like your idea. I'll get the cache size on init and
report the delta at the end of modifying operations (but not block on them).

[jkarlin, 2016-02-10 16:06:33]

Patchset #5 (id:80001) has been deleted

[jkarlin, 2016-02-10 16:24:53]

On 2016/02/10 12:27:03, jkarlin wrote:
> On 2016/02/09 22:23:03, michaeln wrote:
> > can multiple ops be running concurrently? i'm wondering if a change made by
> one
> > op can be double booked by a second op?
> > 
> > start put - size@start = N
> > start delete - size@start = N
> > end delete - size@end = N - D --> deltaReported = -D
> > end put 0 size@end = (N - D) + P --> deltaReported = P - D
> > 
> > If that's possible, a different approach could be to try to attribute
speicif
> > size changes to specific ops. Instead sample how big it is every now and
then,
> > report any delta from last time.
> 
> It's not possible, all ops are run sequentially, for now. But it should be
> possible one day, and I like your idea. I'll get the cache size on init and
> report the delta at the end of modifying operations (but not block on them).

Done. It's much better. I do report the delta before completing the operation as
I don't want to race the CacheStorageCache being freed before the quota manager
gets the update.

[jkarlin, 2016-02-10 16:38:52]

Patchset #5 (id:100001) has been deleted

[jkarlin, 2016-02-10 16:38:59]

Patchset #5 (id:120001) has been deleted

[jkarlin, 2016-02-11 16:49:29]

michaeln: PTAL, thanks!

[michaeln, 2016-02-12 02:06:04]

lgtm (but please see the question)

https://codereview.chromium.org/1681653002/diff/140001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1681653002/diff/140001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:255: :
request(std::move(request)),
nice that this got simpler!

https://codereview.chromium.org/1681653002/diff/140001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1035:
UpdateCacheSize(base::Bind(callback, CACHE_STORAGE_OK));
Do you have to defer completing the put|delete operation until after sampling
the new size or would it be possible to...

callback.Run(CACHE_STORAGE_OK);
UpdateCacheSize(base::Bind(&EmptyCallback));

[jkarlin, 2016-02-12 16:23:01]

Patchset #6 (id:160001) has been deleted

[jkarlin, 2016-02-12 16:33:12]

Thanks!

PTAL at some changes I made in regards to your comment.

https://codereview.chromium.org/1681653002/diff/140001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1681653002/diff/140001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:1035:
UpdateCacheSize(base::Bind(callback, CACHE_STORAGE_OK));
On 2016/02/12 02:06:04, michaeln wrote:
> Do you have to defer completing the put|delete operation until after sampling
> the new size or would it be possible to...
> 
> callback.Run(CACHE_STORAGE_OK);
> UpdateCacheSize(base::Bind(&EmptyCallback));

Done. I didn't earlier because I was concerned about races with the cache being
freed while UpdateCacheSize was running. Fixed that by posting the callback and
keeping a refptr to the cache in UpdateCacheSize.

[michaeln, 2016-02-12 20:16:53]

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:905: FROM_HERE,
base::Bind(put_context->callback, CACHE_STORAGE_OK));
at first i didn't understand why you wanted the PostTask here, i think i do now,
|this| could be deleted during |callback| execution, is that right?

if so, you might be able to reverse the order instead of going thru the msgloop

  UpdateCacheSize();
  callback.Run(OK);

the update method will have safely taken the ref it needs before the callback
has a chance to drop any the callers

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:958: int rv =
backend_->CalculateSizeOfAllEntries(
i was wondering how expensive this was in the backend

uint64_t SimpleIndex::GetCacheSize() const {
  DCHECK(initialized_);
  return cache_size_;
}

nice :)

[jkarlin, 2016-02-12 20:25:49]

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:905: FROM_HERE,
base::Bind(put_context->callback, CACHE_STORAGE_OK));
On 2016/02/12 20:16:53, michaeln wrote:
> at first i didn't understand why you wanted the PostTask here, i think i do
now,
> |this| could be deleted during |callback| execution, is that right?

yes

> 
> if so, you might be able to reverse the order instead of going thru the
msgloop
> 
>   UpdateCacheSize();
>   callback.Run(OK);

MsgLoop is safer.. but yes, this is faster. Done.

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:958: int rv =
backend_->CalculateSizeOfAllEntries(
On 2016/02/12 20:16:53, michaeln wrote:
> i was wondering how expensive this was in the backend
> 
> uint64_t SimpleIndex::GetCacheSize() const {
>   DCHECK(initialized_);
>   return cache_size_;
> }
> 
> nice :)

Definitely. It does posttask to get there though.

[jkarlin, 2016-02-12 20:28:29]

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1681653002/diff/200001/content/browser/cache_...
content/browser/cache_storage/cache_storage_cache.cc:905: FROM_HERE,
base::Bind(put_context->callback, CACHE_STORAGE_OK));
On 2016/02/12 20:25:49, jkarlin wrote:
> On 2016/02/12 20:16:53, michaeln wrote:
> > at first i didn't understand why you wanted the PostTask here, i think i do
> now,
> > |this| could be deleted during |callback| execution, is that right?
> 
> yes
> 
> > 
> > if so, you might be able to reverse the order instead of going thru the
> msgloop
> > 
> >   UpdateCacheSize();
> >   callback.Run(OK);
> 
> MsgLoop is safer.. but yes, this is faster. Done.

By that I meant generally safer. In this case I do believe it's safe.

[jkarlin, 2016-02-12 20:30:47]

Patchset #8 (id:220001) has been deleted

[jkarlin, 2016-02-13 00:18:36]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-02-13 00:18:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1681653002/#ps240001
(title: "Run callbacks immediately")

[commit-bot: I haz the power, 2016-02-13 00:18:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681653002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681653002/240001

[commit-bot: I haz the power, 2016-02-13 00:58:51]

Description was changed from

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

Rather than just report the size of the metadata and the body, report the
difference in cache size from before and after the operation. This fits better
with GetOriginSize which sums cache sizes.

BUG=581709
==========

to

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

Rather than just report the size of the metadata and the body, report the
difference in cache size from before and after the operation. This fits better
with GetOriginSize which sums cache sizes.

BUG=581709
==========

[commit-bot: I haz the power, 2016-02-13 00:58:52]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-02-16 22:46:09]

Description was changed from

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

Rather than just report the size of the metadata and the body, report the
difference in cache size from before and after the operation. This fits better
with GetOriginSize which sums cache sizes.

BUG=581709
==========

to

==========
[CacheStorage] Use backend size to determine change in cache size in put and
delete

Rather than just report the size of the metadata and the body, report the
difference in cache size from before and after the operation. This fits better
with GetOriginSize which sums cache sizes.

BUG=581709

Committed: https://crrev.com/30395012ca331a41f784ce0d129c7eaf4d79f458
Cr-Commit-Position: refs/heads/master@{#375295}
==========

[commit-bot: I haz the power, 2016-02-16 22:46:09]

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