Add a size estimation mechanism to StoragePartitionHttpCacheDataRemover.

Add a size estimation mechanism to StoragePartitionHttpCacheDataRemover.

StoragePartitionHttpCacheDataRemover, the class that enables cache clearing based on time constraints [begin, end), is now able to estimate the size in bytes of entries that would be deleted, without actually deleting them.

The actual computation was so far implemented only for the Blockfile cache backend. Simple and Memory backends to be done in follow-up CLs (but the infrastructure is in place, since disk_cache::Backend is purely virtual).

Update: I missed Backend V3 and testing subclasses. They were added in Patch 3.

Added one browsertest for this functionality.

BUG=510028
Committed: https://crrev.com/aee01cebea1d6de51691a6a525ba958184b7a1f6
Cr-Commit-Position: refs/heads/master@{#352830}

[msramek, 2015-09-01 13:28:05]

Patchset #1 (id:1) has been deleted

[msramek, 2015-09-01 13:34:33]

msramek@chromium.org changed reviewers:
+ gavinp@chromium.org, mkwst@chromium.org

[msramek, 2015-09-01 13:34:33]

Hi Mike and Gavin,

can you please have a look?

Thanks,
Martin

[Mike West, 2015-09-02 11:50:13]

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
File
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc
(right):

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc:196:
// Write 17 bytes of data to the first cache, 6 bytes to the other one.
This seems somewhat arbitrary. Why 23? And why 17 and 6?

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc:216:
EXPECT_TRUE(GetResult() == 23 || GetResult() == net::ERR_NOT_IMPLEMENTED);
Can you add a test which clears the cache (or writes additional data?) and reads
the numbers again to verify they go to 0 and aren't strangely cached?

https://codereview.chromium.org/1304363013/diff/20001/components/browsing_dat...
File components/browsing_data/storage_partition_http_cache_data_remover.cc
(right):

https://codereview.chromium.org/1304363013/diff/20001/components/browsing_dat...
components/browsing_data/storage_partition_http_cache_data_remover.cc:76:
&StoragePartitionHttpCacheDataRemover::ClearHttpCacheOnIOThread,
This feels pretty hacky. Why do we call `::ClearHttpCacheOnIOThread` in order to
count? Why not define a `::CountHttpCacheOnIOThread`?

[msramek, 2015-09-02 19:22:25]

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
File
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc
(right):

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc:110:
if (value > 0) {
Fix: >= 0. Otherwise we would disregard net::OK == 0. Of course, the operation
never finishes immediately, so this wasn't caught.

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc:196:
// Write 17 bytes of data to the first cache, 6 bytes to the other one.
On 2015/09/02 11:50:13, Mike West (buried) wrote:
> This seems somewhat arbitrary. Why 23? And why 17 and 6?

Well, it is arbitrary :) I didn't want to parametrize the helper functions if
they are only used for one test. But now that I'm adding more tests, I'll change
the methods to be more general.

https://codereview.chromium.org/1304363013/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/storage_partition_http_cache_data_remover_browsertest.cc:216:
EXPECT_TRUE(GetResult() == 23 || GetResult() == net::ERR_NOT_IMPLEMENTED);
On 2015/09/02 11:50:13, Mike West (buried) wrote:
> Can you add a test which clears the cache (or writes additional data?) and
reads
> the numbers again to verify they go to 0 and aren't strangely cached?

Done. A lot has changed in this file.

- "PopulatedAndCleared" tests zero -> nonzero -> nonzero
- "Incremental" tests zero -> nonzero -> more nonzero

Additionally,

- "TimeRange" tests time range
- split "Counting" into "Empty" and "Counting"

https://codereview.chromium.org/1304363013/diff/20001/components/browsing_dat...
File components/browsing_data/storage_partition_http_cache_data_remover.cc
(right):

https://codereview.chromium.org/1304363013/diff/20001/components/browsing_dat...
components/browsing_data/storage_partition_http_cache_data_remover.cc:76:
&StoragePartitionHttpCacheDataRemover::ClearHttpCacheOnIOThread,
On 2015/09/02 11:50:13, Mike West (buried) wrote:
> This feels pretty hacky. Why do we call `::ClearHttpCacheOnIOThread` in order
to
> count? Why not define a `::CountHttpCacheOnIOThread`?

I duplicated the methods now. It's less hacky, but, well, duplicated. I also
merged OnCalculationComplete into CountHttpCacheOnIOThread.

https://codereview.chromium.org/1304363013/diff/20001/net/disk_cache/blockfil...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/20001/net/disk_cache/blockfil...
net/disk_cache/blockfile/backend_impl.cc:458: node->DoomImpl();
...and since this method imitates SyncDoomEntriesBetween, I made a very
nonsensical copy-paste error. Caught by the additional tests :)

https://codereview.chromium.org/1304363013/diff/40001/components/browsing_dat...
File components/browsing_data/storage_partition_http_cache_data_remover.cc
(right):

https://codereview.chromium.org/1304363013/diff/40001/components/browsing_dat...
components/browsing_data/storage_partition_http_cache_data_remover.cc:162: if
(delete_begin_.is_null() && delete_end_.is_max()) {
I changed this to check both delete_begin_ and delete_end_. Previously the check
was only on begin, probably because there isn't an entry point that would
support setting delete_end_ != base::Time::Max().

However, I now use both time constraints in the tests.

[Mike West, 2015-09-03 08:19:39]

browsing_data bits LGTM, thanks for taking a second pass.

Also, the compilation error looks relevant. :)

[msramek, 2015-09-03 09:12:58]

On 2015/09/03 08:19:39, Mike West (buried) wrote:
> browsing_data bits LGTM, thanks for taking a second pass.
> 
> Also, the compilation error looks relevant. :)

Thanks, Mike! Yep, it's relevant; there are more subclasses than I thought at
first. Added them in Patch 3.

[msramek, 2015-09-03 09:16:29]

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

[msramek, 2015-09-03 09:16:29]

Gavin, can you have a look at the rest? Alternatively, please start by
confirming whether you're ok with the new methods' signature, so I can avoid
painful rebasing when I move on to implement simple cache and memory cache
counting.

And +Josh PTAL as the owner of cache_storage_cache_unittest.cc.

[jkarlin, 2015-09-03 11:05:53]

Only looked at cache_storage_cache_unittest.cc

FYI: gavinp is out of the office for awhile, you'll likely want to find another
simplecache owner.

https://codereview.chromium.org/1304363013/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1304363013/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:98: return
backend_->DoomEntriesSince(initial_time, end_time, callback);
Hmm, DoomEntriesSince doesn't seem right. Perhaps:

ADD_FAILURE() << "Function not implemented";

[msramek, 2015-09-03 11:22:21]

msramek@chromium.org changed reviewers:
+ pasko@chromium.org, ttuttle@chromium.org
- gavinp@chromium.org

[msramek, 2015-09-03 11:22:22]

Thanks for the info!

-Gavin
+Egor, Thomas, can you please have a look at the cache code?

https://codereview.chromium.org/1304363013/diff/60001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_cache_unittest.cc (right):

https://codereview.chromium.org/1304363013/diff/60001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_cache_unittest.cc:98: return
backend_->DoomEntriesSince(initial_time, end_time, callback);
On 2015/09/03 11:05:53, jkarlin wrote:
> Hmm, DoomEntriesSince doesn't seem right. Perhaps:
> 
> ADD_FAILURE() << "Function not implemented";

We can just propagate the call to |backend_|. I noticed this typo, but
apparently forgot to upload the patch. Sorry for that! :/

[jkarlin, 2015-09-03 11:36:24]

content/browser/cache_storage/ lgtm

[pasko, 2015-09-04 14:11:50]

pasko@chromium.org changed reviewers:
+ rvargas@chromium.org

[pasko, 2015-09-04 14:11:50]

sorry for late response. I want to also hear the opinion from rvargas@ about
changing the disk cache api. Sorry for that too.

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:198: virtual int GetEntrySize() const = 0;
I think it is a _bad_ interface to use for calculating overall size of the
cache.

This suggests that all entries in the cache should be opened one by one to get
the overall size. That can be very inefficient (it *is* inefficient in
simplecache compared to how SimpleIndex would do it).

Can we avoid exposing this and have only new interface in the backend exposed?

If a (deficient) backend can only calculate size by opening everything, it has a
way to talk to EntryImpl/whatever directly.

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:477: // TODO(msramek): Implement.
for implementation I would suggest to wait for index to get ready:
index_->ExecuteWhenReady() then just go through all 
entries_set_ and ask for GetEntrySize() there.

[msramek, 2015-09-04 16:56:47]

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:198: virtual int GetEntrySize() const = 0;
On 2015/09/04 14:11:50, pasko wrote:
> I think it is a _bad_ interface to use for calculating overall size of the
> cache.
> 
> This suggests that all entries in the cache should be opened one by one to get
> the overall size. That can be very inefficient (it *is* inefficient in
> simplecache compared to how SimpleIndex would do it).
> 
> Can we avoid exposing this and have only new interface in the backend exposed?
> 
> If a (deficient) backend can only calculate size by opening everything, it has
a
> way to talk to EntryImpl/whatever directly.

Makes sense. I removed GetEntrySize from everywhere except EntryImpl.

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:477: // TODO(msramek): Implement.
On 2015/09/04 14:11:50, pasko wrote:
> for implementation I would suggest to wait for index to get ready:
> index_->ExecuteWhenReady() then just go through all 
> entries_set_ and ask for GetEntrySize() there.

Acknowledged. But if you don't mind, I'll do that in a followup CL. I'll make
sure to include you as a reviewer.

(I want to do each backend separately, but I had to put the infrastructure in
place first)

[msramek, 2015-09-04 16:58:01]

On 2015/09/04 14:11:50, pasko wrote:
> sorry for late response. I want to also hear the opinion from rvargas@ about
> changing the disk cache api. Sorry for that too.
> 

Yep, that's ok. It's good to have more pairs of eyes on this. Of course I don't
want to mess up something that would make Chrome slower :)

[pasko, 2015-09-08 13:13:49]

net/disk_cache/simple/ ltgm
invoke rvargas@ for net/disk_cache/disk_cache.h

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:477: // TODO(msramek): Implement.
On 2015/09/04 16:56:47, msramek wrote:
> On 2015/09/04 14:11:50, pasko wrote:
> > for implementation I would suggest to wait for index to get ready:
> > index_->ExecuteWhenReady() then just go through all 
> > entries_set_ and ask for GetEntrySize() there.
> 
> Acknowledged. But if you don't mind, I'll do that in a followup CL. I'll make
> sure to include you as a reviewer.
> 
> (I want to do each backend separately, but I had to put the infrastructure in
> place first)

sure, sorry for the confusion, I only wrote it to help with future CL(s)

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:152: // completes.
Please also make it similar to DoomEntriesBetween with this portion of comment:
This supports unbounded time ranges in either direction by using null Time
values for either argument.

[Deprecated (see juliatuttle), 2015-09-08 17:44:40]

Overall approach looks fine; I'd defer to rvargas for the correctness of the
implementation for the blockfile cache.

I'd be happy to implement it for Simple Cache, or gavinp could (but he may be on
leave).

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/blockfi...
net/disk_cache/blockfile/backend_impl.cc:442: DCHECK_GE(end_time, initial_time);
Does this work even if one or both of initial and end time are null, which is
allowed as an open-ended interval? (I believe null is represented as 0, and
therefore a finite initial time with no end time might trip this DCHECK?)

[msramek, 2015-09-08 18:32:23]

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/blockfi...
net/disk_cache/blockfile/backend_impl.cc:442: DCHECK_GE(end_time, initial_time);
On 2015/09/08 17:44:40, ttuttle wrote:
> Does this work even if one or both of initial and end time are null, which is
> allowed as an open-ended interval? (I believe null is represented as 0, and
> therefore a finite initial time with no end time might trip this DCHECK?)

If you look at BackendImpl::SyncDoomEntriesBetween, the same check is on line
382 above. Furthermore, according to lines 397 and 398, deletion with two 0
parameters never worked.

(where by 0 I mean uninitialized base::Time() = 1st January 1601; it's not
actually NULL, because these are not pointers)

The current situation is that
1. BrowsingDataRemover uses [base::Time(), base::Time::Max()) to represent
unbounded interval

2. StorageHttpCacheDataRemover gets the interval from BrowsingDataRemover and
decides whether to use DoomAllEntries() or DoomEntriesBetween().

3. disk_cache should use DoomAllEntries() for unbounded interval. Less effective
DoomEntriesBetween(base::Time(), base::Time::Max()) also works. But
DoomEntriesBetween(base::Time(), base::Time()) doesn't.

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:152: // completes.
On 2015/09/08 13:13:49, pasko wrote:
> Please also make it similar to DoomEntriesBetween with this portion of
comment:
> This supports unbounded time ranges in either direction by using null Time
> values for either argument.

As explained in the other comment, this part is apparently not true. I suggest
omitting the part, as this method should actually not be used for unbounded
deletion at all.

[pasko, 2015-09-09 09:09:07]

On 2015/09/08 13:13:49, pasko wrote:
> net/disk_cache/simple/ ltgm

I meant lgtm, sorry

[pasko, 2015-09-09 09:09:29]

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:152: // completes.
On 2015/09/08 18:32:22, msramek wrote:
> On 2015/09/08 13:13:49, pasko wrote:
> > Please also make it similar to DoomEntriesBetween with this portion of
> comment:
> > This supports unbounded time ranges in either direction by using null Time
> > values for either argument.
> 
> As explained in the other comment, this part is apparently not true. I suggest
> omitting the part, as this method should actually not be used for unbounded
> deletion at all.

opps, for simple_cache we certainly allow unbounded dooming. For
DoomAllEntries(...) the backend just calls DoomEntriesBetween(Time(), Time(),
...).

Where is the other comment you refer to? Can we clarify that for both functions
then? I am confused otherwise.

[msramek, 2015-09-09 09:20:42]

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:152: // completes.
On 2015/09/09 09:09:29, pasko wrote:
> On 2015/09/08 18:32:22, msramek wrote:
> > On 2015/09/08 13:13:49, pasko wrote:
> > > Please also make it similar to DoomEntriesBetween with this portion of
> > comment:
> > > This supports unbounded time ranges in either direction by using null Time
> > > values for either argument.
> > 
> > As explained in the other comment, this part is apparently not true. I
suggest
> > omitting the part, as this method should actually not be used for unbounded
> > deletion at all.
> 
> opps, for simple_cache we certainly allow unbounded dooming. For
> DoomAllEntries(...) the backend just calls DoomEntriesBetween(Time(), Time(),
> ...).
> 
> Where is the other comment you refer to? Can we clarify that for both
functions
> then? I am confused otherwise.

The comment is in Patchset 6, backend_impl.cc.

Interesting! In the Blockfile backend, DoomAllEntries simply purges the entire
cache, while DoomEntriesBetween goes element by element and tests the time
range.

In Simple backend, I assume that you implemented the semantics that (Time(),
Time()) means unbounded interval because the comment in disk_cache.h said so.

I would suggest to consolidate this to [base::Time(), base::Time::Max()). The
reasons are:

1. BrowsingDataRemover uses this to represent unbounded interval.
2. You can always do a simple check if begin_time < current_time < end_time
without any special cases.
3. It stays hidden from the caller anyway, because the caller should call
DoomAllEntries(). And how exactly this is passed to DoomEntriesBetween() is an
implementation detail.

[pasko, 2015-09-10 18:40:22]

On 2015/09/09 09:20:42, msramek wrote:
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> File net/disk_cache/disk_cache.h (right):
> 
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> net/disk_cache/disk_cache.h:152: // completes.
> On 2015/09/09 09:09:29, pasko wrote:
> > On 2015/09/08 18:32:22, msramek wrote:
> > > On 2015/09/08 13:13:49, pasko wrote:
> > > > Please also make it similar to DoomEntriesBetween with this portion of
> > > comment:
> > > > This supports unbounded time ranges in either direction by using null
Time
> > > > values for either argument.
> > > 
> > > As explained in the other comment, this part is apparently not true. I
> suggest
> > > omitting the part, as this method should actually not be used for
unbounded
> > > deletion at all.
> > 
> > opps, for simple_cache we certainly allow unbounded dooming. For
> > DoomAllEntries(...) the backend just calls DoomEntriesBetween(Time(),
Time(),
> > ...).
> > 
> > Where is the other comment you refer to? Can we clarify that for both
> functions
> > then? I am confused otherwise.
> 
> The comment is in Patchset 6, backend_impl.cc.

It says:
"""
disk_cache should use DoomAllEntries() for unbounded interval. Less effective
DoomEntriesBetween(base::Time(), base::Time::Max()) also works. But
DoomEntriesBetween(base::Time(), base::Time()) doesn't.
"""

I don't understand this assertion and I don't see a connection (maybe it's
late).

DoomEntriesBetween certainly treats base::Time() (which returns true for
is_null()) in end_time as infinity:
https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/blo...

> Interesting! In the Blockfile backend, DoomAllEntries simply purges the entire
> cache, while DoomEntriesBetween goes element by element and tests the time
> range.

I assume DoomAllEntries() purges the entire cache as an optimization, and not
100% equivalent to DoomEntriesBetween(base::Time(), base::Time()), but the
effects are probably not distinguishable by disk_cache API users. For
SimpleCache I just did not want to optimize prematurely, it has DoomAllEntries()
only for compat reasons.

> In Simple backend, I assume that you implemented the semantics that (Time(),
> Time()) means unbounded interval because the comment in disk_cache.h said so.

Probably.

> I would suggest to consolidate this to [base::Time(), base::Time::Max()). The
> reasons are:
> 
> 1. BrowsingDataRemover uses this to represent unbounded interval.
> 2. You can always do a simple check if begin_time < current_time < end_time
> without any special cases.
> 3. It stays hidden from the caller anyway, because the caller should call
> DoomAllEntries(). And how exactly this is passed to DoomEntriesBetween() is an
> implementation detail.

I don't think there is much reason for the caller to bother with choosing
DoomAllEntries() properly. Why copying the same code across clients when the
backend could do the same thing in one place? IMHO DoomAllEntries is not needed
in the API at all, but having it is up to rvargas@, and I am going to accept
whatever his decision there is on this detail, not a big deal.

[msramek, 2015-09-10 18:54:48]

On 2015/09/10 18:40:22, pasko wrote:
> On 2015/09/09 09:20:42, msramek wrote:
> >
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> > File net/disk_cache/disk_cache.h (right):
> > 
> >
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> > net/disk_cache/disk_cache.h:152: // completes.
> > On 2015/09/09 09:09:29, pasko wrote:
> > > On 2015/09/08 18:32:22, msramek wrote:
> > > > On 2015/09/08 13:13:49, pasko wrote:
> > > > > Please also make it similar to DoomEntriesBetween with this portion of
> > > > comment:
> > > > > This supports unbounded time ranges in either direction by using null
> Time
> > > > > values for either argument.
> > > > 
> > > > As explained in the other comment, this part is apparently not true. I
> > suggest
> > > > omitting the part, as this method should actually not be used for
> unbounded
> > > > deletion at all.
> > > 
> > > opps, for simple_cache we certainly allow unbounded dooming. For
> > > DoomAllEntries(...) the backend just calls DoomEntriesBetween(Time(),
> Time(),
> > > ...).
> > > 
> > > Where is the other comment you refer to? Can we clarify that for both
> > functions
> > > then? I am confused otherwise.
> > 
> > The comment is in Patchset 6, backend_impl.cc.
> 
> It says:
> """
> disk_cache should use DoomAllEntries() for unbounded interval. Less effective
> DoomEntriesBetween(base::Time(), base::Time::Max()) also works. But
> DoomEntriesBetween(base::Time(), base::Time()) doesn't.
> """
> 
> I don't understand this assertion and I don't see a connection (maybe it's
> late).
> 
> DoomEntriesBetween certainly treats base::Time() (which returns true for
> is_null()) in end_time as infinity:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/blo...

Ah! You're right. DoomEntriesBetween() redirects to DoomAllEntries() right in
the beginning. I was only looking at the check in the for-loop below (it was
also an evening... ).

I must revert Patchset 7.

> 
> > Interesting! In the Blockfile backend, DoomAllEntries simply purges the
entire
> > cache, while DoomEntriesBetween goes element by element and tests the time
> > range.
> 
> I assume DoomAllEntries() purges the entire cache as an optimization, and not
> 100% equivalent to DoomEntriesBetween(base::Time(), base::Time()), but the
> effects are probably not distinguishable by disk_cache API users. For
> SimpleCache I just did not want to optimize prematurely, it has
DoomAllEntries()
> only for compat reasons.
> 
> > In Simple backend, I assume that you implemented the semantics that (Time(),
> > Time()) means unbounded interval because the comment in disk_cache.h said
so.
> 
> Probably.
> 
> > I would suggest to consolidate this to [base::Time(), base::Time::Max()).
The
> > reasons are:
> > 
> > 1. BrowsingDataRemover uses this to represent unbounded interval.
> > 2. You can always do a simple check if begin_time < current_time < end_time
> > without any special cases.
> > 3. It stays hidden from the caller anyway, because the caller should call
> > DoomAllEntries(). And how exactly this is passed to DoomEntriesBetween() is
an
> > implementation detail.
> 
> I don't think there is much reason for the caller to bother with choosing
> DoomAllEntries() properly. Why copying the same code across clients when the
> backend could do the same thing in one place? IMHO DoomAllEntries is not
needed
> in the API at all, but having it is up to rvargas@, and I am going to accept
> whatever his decision there is on this detail, not a big deal.

Great that you opened this topic. I was just about to ask the same question - it
would probably be valuable to optimize counting of the infinite data range - and
I don't know if it's preferred to have a separate function, or if I should just
add it to the existing range-parametrized function.

[rvargas (doing something else), 2015-09-12 00:13:02]

net looks fine, but we need unit tests.

Also, I wonder if you saw my comment on the bug.

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
net/disk_cache/blockfile/backend_impl.cc:456: return net::OK;
nit: This would be 0 (a size).

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:151: // The return value is the number of bytes
written or a net error code.
bytes written?

[rvargas (doing something else), 2015-09-12 00:24:03]

On 2015/09/10 18:54:48, msramek wrote:
> On 2015/09/10 18:40:22, pasko wrote:
> > On 2015/09/09 09:20:42, msramek wrote:
> > >
> >
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> > > File net/disk_cache/disk_cache.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1304363013/diff/120001/net/disk_cache/disk_ca...
> > > net/disk_cache/disk_cache.h:152: // completes.
> > > On 2015/09/09 09:09:29, pasko wrote:
> > > > On 2015/09/08 18:32:22, msramek wrote:
> > > > > On 2015/09/08 13:13:49, pasko wrote:
> > > > > > Please also make it similar to DoomEntriesBetween with this portion
of
> > > > > comment:
> > > > > > This supports unbounded time ranges in either direction by using
null
> > Time
> > > > > > values for either argument.
> > > > > 
> > > > > As explained in the other comment, this part is apparently not true. I
> > > suggest
> > > > > omitting the part, as this method should actually not be used for
> > unbounded
> > > > > deletion at all.
> > > > 
> > > > opps, for simple_cache we certainly allow unbounded dooming. For
> > > > DoomAllEntries(...) the backend just calls DoomEntriesBetween(Time(),
> > Time(),
> > > > ...).
> > > > 
> > > > Where is the other comment you refer to? Can we clarify that for both
> > > functions
> > > > then? I am confused otherwise.
> > > 
> > > The comment is in Patchset 6, backend_impl.cc.
> > 
> > It says:
> > """
> > disk_cache should use DoomAllEntries() for unbounded interval. Less
effective
> > DoomEntriesBetween(base::Time(), base::Time::Max()) also works. But
> > DoomEntriesBetween(base::Time(), base::Time()) doesn't.
> > """
> > 
> > I don't understand this assertion and I don't see a connection (maybe it's
> > late).
> > 
> > DoomEntriesBetween certainly treats base::Time() (which returns true for
> > is_null()) in end_time as infinity:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/disk_cache/blo...
> 
> Ah! You're right. DoomEntriesBetween() redirects to DoomAllEntries() right in
> the beginning. I was only looking at the check in the for-loop below (it was
> also an evening... ).
> 
> I must revert Patchset 7.
> 
> > 
> > > Interesting! In the Blockfile backend, DoomAllEntries simply purges the
> entire
> > > cache, while DoomEntriesBetween goes element by element and tests the time
> > > range.
> > 
> > I assume DoomAllEntries() purges the entire cache as an optimization, and
not
> > 100% equivalent to DoomEntriesBetween(base::Time(), base::Time()), but the
> > effects are probably not distinguishable by disk_cache API users. For
> > SimpleCache I just did not want to optimize prematurely, it has
> DoomAllEntries()
> > only for compat reasons.
> > 
> > > In Simple backend, I assume that you implemented the semantics that
(Time(),
> > > Time()) means unbounded interval because the comment in disk_cache.h said
> so.
> > 
> > Probably.
> > 
> > > I would suggest to consolidate this to [base::Time(), base::Time::Max()).
> The
> > > reasons are:
> > > 
> > > 1. BrowsingDataRemover uses this to represent unbounded interval.
> > > 2. You can always do a simple check if begin_time < current_time <
end_time
> > > without any special cases.
> > > 3. It stays hidden from the caller anyway, because the caller should call
> > > DoomAllEntries(). And how exactly this is passed to DoomEntriesBetween()
is
> an
> > > implementation detail.
> > 
> > I don't think there is much reason for the caller to bother with choosing
> > DoomAllEntries() properly. Why copying the same code across clients when the
> > backend could do the same thing in one place? IMHO DoomAllEntries is not
> needed
> > in the API at all, but having it is up to rvargas@, and I am going to accept
> > whatever his decision there is on this detail, not a big deal.
> 
> Great that you opened this topic. I was just about to ask the same question -
it
> would probably be valuable to optimize counting of the infinite data range -
and
> I don't know if it's preferred to have a separate function, or if I should
just
> add it to the existing range-parametrized function.

The Doom API was actually pushed from the consumer (browsing data remover), and
it can certainly change. There should be a bug filed a while ago to remove
things that are not in use. (like two different methods to do ranges). Having a
single entry point sounds fine to me.

From an implementation point of view, iteration can be quite slow, which is part
of the reason I asked on the bug about this change. I think it's better to omit
the data if it cannot be retrieved very quickly.

[msramek, 2015-09-15 11:42:36]

Well, I agree that it makes sense to start with a simpler implementation and add
the time ranges when we need them. My problem is that we need them now.

The current design that was agreed upon contains the cache estimation for all
time ranges, not just for the unlimited range. We already discussed special
treatment for the unlimited range count for history, but the UX weren't happy
about the inconsistency. So even for cache, we want at least V1 to contain a
fully-functioning cache counting.

What I don't understand from your perspective is that the complexity is not new;
this is the very same code as in DoomEntries, just in a form of a "dry run".
(Hm! Would it maybe be better to actually parametrize DoomEntries by a boolean
instead?)

Regarding the time necessary for the computation, it is acceptable - the
counting will not happen continuously, it will only start after a user's action
(open the CBD dialog and check cache), it will be done asynchronously (as are
the other counters as well) and it's OK if it takes some time (it's a
supplementary information that will be shown to the user when available).
Furthermore, since it's done in the context of the CBD dialog, we can assume
that blocking the cache by this heavy operation is not a problem, since the user
is not actively browsing and in fact, is about to trigger the even heavier
operation DoomEntries.

That said, I cannot guarantee that this is valuable until we actually try it
out. I'll gladly simplify the code if we find out it's not needed, and if you
are concerned about BrowsingDataRemover already complicating the code by
unnecessary API, I'm also happy to help with removing the superfluous DoomEntry
variants now that I work in this area.

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/backend_impl.cc (right):

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
net/disk_cache/blockfile/backend_impl.cc:456: return net::OK;
On 2015/09/12 00:13:02, rvargas (slow to respond) wrote:
> nit: This would be 0 (a size).

Done.

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:151: // The return value is the number of bytes
written or a net error code.
On 2015/09/12 00:13:02, rvargas (slow to respond) wrote:
> bytes written?

Done. Sorry, copy-paste.

[rvargas (doing something else), 2015-09-15 19:04:13]

On 2015/09/15 11:42:36, msramek wrote:
> Well, I agree that it makes sense to start with a simpler implementation and
add
> the time ranges when we need them. My problem is that we need them now.
> 
> The current design that was agreed upon contains the cache estimation for all
> time ranges, not just for the unlimited range. We already discussed special
> treatment for the unlimited range count for history, but the UX weren't happy
> about the inconsistency. So even for cache, we want at least V1 to contain a
> fully-functioning cache counting.

I know that UX issues are generally not treated that way, but it would be nice
to document that kind of stuff on the bug tracker... it's quite hard to figure
out later why are we doing what are we doing (what were we trying to accomplish
with the design etc). After people move away, we end up relying on vague
recollections of people not directly involved on the decisions.

> What I don't understand from your perspective is that the complexity is not
new;
> this is the very same code as in DoomEntries, just in a form of a "dry run".
> (Hm! Would it maybe be better to actually parametrize DoomEntries by a boolean
> instead?)

The code itself is not too bad: it is short and easy enough to understand so it
doesn't really concern me. The API touches a lot of places, which is not ideal,
but it's not too bad either; I think it's better than trying to pack everything
into a single method.

> 
> Regarding the time necessary for the computation, it is acceptable - the
> counting will not happen continuously, it will only start after a user's
action
> (open the CBD dialog and check cache), it will be done asynchronously (as are
> the other counters as well) and it's OK if it takes some time (it's a
> supplementary information that will be shown to the user when available).
> Furthermore, since it's done in the context of the CBD dialog, we can assume
> that blocking the cache by this heavy operation is not a problem, since the
user
> is not actively browsing and in fact, is about to trigger the even heavier
> operation DoomEntries.

Actually, it may be the case that the time to calculate the size through an
enumeration may be close to the time it would take to delete the entries.
(caveat, the dry run should make deletion work way faster)... but deletion can
take a very long time, depending on how many entries are deleted and when was
the last time that they were used. I won't be surprised with numbers above 10
seconds. I will not expect users to way for 10 seconds on that dialog without
taking an actual action, so we may not show any number at all. (We should
measure actual times).

Worst of all, we call the new function, which will not be aborted and has to
complete before we start again the enumeration to actually delete the entries.

I doubt that was considered when agreeing on the functionality.
 
> That said, I cannot guarantee that this is valuable until we actually try it
> out. I'll gladly simplify the code if we find out it's not needed, and if you

How would we really know if the data is valuable at all?. I've seen proposals
about data driven UI changes lead nowhere :(

As I said on the bug, I'm missing all the context here, but it's hard for me to
go from the objectives that I can see to the actual change (as in, the
enumeration will never be called on mobile).

> are concerned about BrowsingDataRemover already complicating the code by
> unnecessary API, I'm also happy to help with removing the superfluous
DoomEntry
> variants now that I work in this area.

Thanks!. I think's that's kind of orthogonal to this change so I would not tie
them together (but it touches pretty much the same files).

BTW, if we end up with only one method, wouldn't that be DoomEntriesSince()?. I
mean, that's what the UI asks for.

> 
>
https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
> File net/disk_cache/blockfile/backend_impl.cc (right):
> 
>
https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/blockfi...
> net/disk_cache/blockfile/backend_impl.cc:456: return net::OK;
> On 2015/09/12 00:13:02, rvargas (slow to respond) wrote:
> > nit: This would be 0 (a size).
> 
> Done.
> 
>
https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
> File net/disk_cache/disk_cache.h (right):
> 
>
https://codereview.chromium.org/1304363013/diff/180001/net/disk_cache/disk_ca...
> net/disk_cache/disk_cache.h:151: // The return value is the number of bytes
> written or a net error code.
> On 2015/09/12 00:13:02, rvargas (slow to respond) wrote:
> > bytes written?
> 
> Done. Sorry, copy-paste.

[pasko, 2015-09-15 19:22:38]

On 2015/09/15 19:04:13, rvargas (slow to respond) wrote:
> On 2015/09/15 11:42:36, msramek wrote:
> > Well, I agree that it makes sense to start with a simpler implementation and
> add
> > the time ranges when we need them. My problem is that we need them now.
> > 
> > The current design that was agreed upon contains the cache estimation for
all
> > time ranges, not just for the unlimited range. We already discussed special
> > treatment for the unlimited range count for history, but the UX weren't
happy
> > about the inconsistency. So even for cache, we want at least V1 to contain a
> > fully-functioning cache counting.
> 
> I know that UX issues are generally not treated that way, but it would be nice
> to document that kind of stuff on the bug tracker... it's quite hard to figure
> out later why are we doing what are we doing (what were we trying to
accomplish
> with the design etc). After people move away, we end up relying on vague
> recollections of people not directly involved on the decisions.
> 
> > What I don't understand from your perspective is that the complexity is not
> new;
> > this is the very same code as in DoomEntries, just in a form of a "dry run".
> > (Hm! Would it maybe be better to actually parametrize DoomEntries by a
boolean
> > instead?)
> 
> The code itself is not too bad: it is short and easy enough to understand so
it
> doesn't really concern me. The API touches a lot of places, which is not
ideal,
> but it's not too bad either; I think it's better than trying to pack
everything
> into a single method.
> 
> > 
> > Regarding the time necessary for the computation, it is acceptable - the
> > counting will not happen continuously, it will only start after a user's
> action
> > (open the CBD dialog and check cache), it will be done asynchronously (as
are
> > the other counters as well) and it's OK if it takes some time (it's a
> > supplementary information that will be shown to the user when available).
> > Furthermore, since it's done in the context of the CBD dialog, we can assume
> > that blocking the cache by this heavy operation is not a problem, since the
> user
> > is not actively browsing and in fact, is about to trigger the even heavier
> > operation DoomEntries.
> 
> Actually, it may be the case that the time to calculate the size through an
> enumeration may be close to the time it would take to delete the entries.
> (caveat, the dry run should make deletion work way faster)... but deletion can
> take a very long time, depending on how many entries are deleted and when was
> the last time that they were used. I won't be surprised with numbers above 10
> seconds. I will not expect users to way for 10 seconds on that dialog without
> taking an actual action, so we may not show any number at all. (We should
> measure actual times).

2c: UMA observed caches of 200000 entries (two hundred thousand). If we spend
one disk seek per cache entry for size calculation on slow devices, we would get
results back in 30 minutes, and this is when the rest of the system is not busy.

Are there any UI mocks that utilize this range-based size calculations?

[msramek, 2015-09-23 13:10:45]

Patchset #11 (id:220001) has been deleted

[msramek, 2015-09-23 13:38:12]

(sorry for the delay, I was OOO a few days)

Thanks for your patience, guys, I am convinced now :) I'll start with the
implementation for the size of the entire cache, and revisit the UX discussions
regarding a) consistency between time ranges and b) the value of the calculation
if the user needs to wait for a long time. In the meantime, I'll try to add some
metrics about deletion time to use them as estimates for counting time.

Egor, the mocks do not show range-based calculation exactly because it was
agreed that we will behave the same way for any range selection. I understand
the complaints that the bug doesn't contain all context. I wanted to link at
least the email thread, but it actually wasn't on a mailinglist... I'll update
the bug with details when I have a new specification.

As for the technical problem that Ricardo pointed out, we should be able to tell
the counting method on the IO thread to bail at any time during the iteration so
that it doesn't block the deletion. But that is also beyond the scope of this
CL. I'll think about this in case we decide to proceed with a full counting
functionality.

I uploaded a new patch, but the tests fail; apparently IndexHeader::num_bytes
does not have the value I thought it did. Am I supposed to do something else
before I can take this value from the header? (that's why I didn't remove the
InFlightBackendIO code yet). Or is this only the size of the header file? (but
the values would still seem to low; I'm basically getting values on the order of
IndexHeader::num_entries)

[msramek, 2015-09-23 13:39:52]

> > are concerned about BrowsingDataRemover already complicating the code by
> > unnecessary API, I'm also happy to help with removing the superfluous
> DoomEntry
> > variants now that I work in this area.
> 
> Thanks!. I think's that's kind of orthogonal to this change so I would not tie
> them together (but it touches pretty much the same files).
> 
> BTW, if we end up with only one method, wouldn't that be DoomEntriesSince()?.
I
> mean, that's what the UI asks for.
> 

It should still be DoomEntriesBetween. BrowsingDataRemover uses two time
constraints (i.e. "Between"), and most of our storage backends support that.
Only several storage backends currently support only one time constraint (i.e.
"Since"), and we plan to improve this soon. See crbug.com/449639 and
crbug.com/529294.

[rvargas (doing something else), 2015-09-24 00:14:45]

On 2015/09/23 13:38:12, msramek wrote:
> (sorry for the delay, I was OOO a few days)
> 
> Thanks for your patience, guys, I am convinced now :) I'll start with the
> implementation for the size of the entire cache, and revisit the UX
discussions
> regarding a) consistency between time ranges and b) the value of the
calculation
> if the user needs to wait for a long time. In the meantime, I'll try to add
some
> metrics about deletion time to use them as estimates for counting time.
> 
> Egor, the mocks do not show range-based calculation exactly because it was
> agreed that we will behave the same way for any range selection. I understand
> the complaints that the bug doesn't contain all context. I wanted to link at
> least the email thread, but it actually wasn't on a mailinglist... I'll update
> the bug with details when I have a new specification.
> 
> As for the technical problem that Ricardo pointed out, we should be able to
tell
> the counting method on the IO thread to bail at any time during the iteration
so
> that it doesn't block the deletion. But that is also beyond the scope of this
> CL. I'll think about this in case we decide to proceed with a full counting
> functionality.
> 
> I uploaded a new patch, but the tests fail; apparently IndexHeader::num_bytes
> does not have the value I thought it did. Am I supposed to do something else
> before I can take this value from the header? (that's why I didn't remove the
> InFlightBackendIO code yet). Or is this only the size of the header file? (but
> the values would still seem to low; I'm basically getting values on the order
of
> IndexHeader::num_entries)

I missed the email :(

The code looks correct to me. BTW, when I said that we need unit tests I was
thinking about net_unittests (as in the new API has to be tested at that layer).

Take a look at backend_unittest.cc. num_bytes should be updated after
entry->Close(). You probably don't need any call to FlushQueueForTest() because
CalculateSizeOfAllEntries() will push things through the queues. Note that the
reported size will also include the size of the keys (this may be backend
dependent).

so
EXPECT_EX(x, WriteData(entry,...x))
entry->Close();
CalculateSizeOfAllEntries(cb.callback());
EXPECT_EQ(x + key_len, cb.GetResult(rv));

should work. I don't know why your current test would fail.

[pasko, 2015-09-25 11:47:40]

New design and current changes in net/disk_cache/simple/ ltgm
thanks for making future reading simpler!

When writing unittests (I'm ok to do it later when the actual implementation
appears), please don't forget to test all backends:
For example, there is a useful function SetSimpleCacheMode().

[msramek, 2015-09-29 09:53:42]

Eh, it again took me a while to get back to this CL...

Thanks for the hint! I suspected that I just need to flush something somewhere
and yes, entry->Close() was what was missing in my tests.

Anyway, I moved the test to backend_unittest.cc. It's also much shorter than it
originally was, as it appears I originally rewrote some of the test
infrastructure that you already had...

Please take another look!

Thanks,
Martin

[rvargas (doing something else), 2015-09-30 17:04:42]

PS 11 lgtm modulo nit.

https://codereview.chromium.org/1304363013/diff/300001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1304363013/diff/300001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1741: EXPECT_EQ(result + last_entry_size +
(int)key.size(), new_result);
nit: static_cast.

[pasko, 2015-10-02 09:48:42]

should this be in CQ already? :)

[msramek, 2015-10-06 10:55:29]

Yes, it should! I was on an offsite :)

https://codereview.chromium.org/1304363013/diff/300001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/1304363013/diff/300001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:1741: EXPECT_EQ(result + last_entry_size +
(int)key.size(), new_result);
On 2015/09/30 17:04:41, rvargas (doing something else) wrote:
> nit: static_cast.

Done.

[msramek, 2015-10-06 10:55:40]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-10-06 10:55:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
jkarlin@chromium.org, pasko@chromium.org, rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1304363013/#ps320001
(title: "static_cast")

[commit-bot: I haz the power, 2015-10-06 10:55:57]

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

[commit-bot: I haz the power, 2015-10-06 11:06:15]

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

[commit-bot: I haz the power, 2015-10-06 11:06:16]

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...)

[msramek, 2015-10-06 11:17:42]

Oh, I see that rvargas@ removed himself from OWNERS in the meantime.

ttuttle@, could you please rubberstamp as another net/ owner?

[Deprecated (see juliatuttle), 2015-10-07 12:18:55]

On 2015/10/06 11:17:42, msramek wrote:
> Oh, I see that rvargas@ removed himself from OWNERS in the meantime.
> 
> ttuttle@, could you please rubberstamp as another net/ owner?

rubber-stamp lgtm based on rvargas' lgtm.

cheers!

[msramek, 2015-10-07 12:22:02]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2015-10-07 12:22:50]

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

[commit-bot: I haz the power, 2015-10-07 14:23:43]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2015-10-07 14:25:00]

Patchset 15 (id:??) landed as
https://crrev.com/aee01cebea1d6de51691a6a525ba958184b7a1f6
Cr-Commit-Position: refs/heads/master@{#352830}