Interface for associating in-memory hint bytes with disk cache entries.

Interface for associating in-memory hint bytes with disk cache entries.
Implementation for it in the mock cache (with off switch)

The intent is to implement it in simple cache, and to use it in
HttpCache::Transaction to avoid opening entries that would not be usable
under present load flags due to caching-hostile headers.

BUG=729679
Review-Url: https://codereview.chromium.org/2958033002
Cr-Commit-Position: refs/heads/master@{#498491}
Committed: https://chromium.googlesource.com/chromium/src/+/e63bba9024d3397e80e393aed842ba9957385796

[Maks Orlovich, 2017-06-27 15:30:02]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 15:30:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-06-27 15:34:44]

morlovich@chromium.org changed reviewers:
+ gavinp@chromium.org, pasko@chromium.org, rdsmith@chromium.org

[Maks Orlovich, 2017-06-27 15:34:45]

Piece #1 of https://codereview.chromium.org/2922973003/

[Randy Smith (Not in Mondays), 2017-06-27 15:37:17]

On 2017/06/27 15:34:45, Maks Orlovich wrote:
> Piece #1 of https://codereview.chromium.org/2922973003/

Maks: Do you need all of us reviewing this?  As noted earlier, I'm inclined to
stay out unless there's something specific you think I'd be useful on--I think
you have better reviewers than I at the implementation level for all parts of
this CL.

[Maks Orlovich, 2017-06-27 15:45:15]

> Maks: Do you need all of us reviewing this?  As noted earlier, I'm inclined to
> stay out unless there's something specific you think I'd be useful on--I think
> you have better reviewers than I at the implementation level for all parts of
> this CL.

Not strongly, but this one is basically an interface CL --- the implementation
in the 
mock is completely uninteresting --- so I felt like you may have opinions on any
design back-and-forth,
especially since Gavin and Pasko seem to have different opinion on what the
getter should look like, 
and I don't care strongly either way...

Based on timing of things (given your vacation and Josh seemingly busy?), I may
or may not ask your for the
HttpCache::Transaction piece, but I certainly won't ask you for the SimpleCache
impl piece.

[Randy Smith (Not in Mondays), 2017-06-27 15:59:37]

On 2017/06/27 15:45:15, Maks Orlovich wrote:
> > Maks: Do you need all of us reviewing this?  As noted earlier, I'm inclined
to
> > stay out unless there's something specific you think I'd be useful on--I
think
> > you have better reviewers than I at the implementation level for all parts
of
> > this CL.
> 
> Not strongly, but this one is basically an interface CL --- the implementation
> in the 
> mock is completely uninteresting --- so I felt like you may have opinions on
any
> design back-and-forth,
> especially since Gavin and Pasko seem to have different opinion on what the
> getter should look like, 
> and I don't care strongly either way...
> 
> Based on timing of things (given your vacation and Josh seemingly busy?), I
may
> or may not ask your for the
> HttpCache::Transaction piece, but I certainly won't ask you for the
SimpleCache
> impl piece.

SG, just wanted to make sure you were specifically targeting me rather than
sending reviews broadly.  I'll put it on my queue.  Thanks!

[commit-bot: I haz the power, 2017-06-27 16:45:58]

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

[commit-bot: I haz the power, 2017-06-27 16:45:58]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-06-27 21:33:56]

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:193: // - If the entry is not in the cache, returns
0.
Definitively isn't in the cache, or isn't in the in-memory index associated with
the cache?  I.e. is this case a "Maybe there's an entry with this key--I don't
know" or "There is no entry with this key in the cache"?

The reason I ask is that the ability to be sure that there isn't an entry with
this key in the cache without checking disk, even in a best effort fashion, is a
simple cache feature.  If we want to leave the door open to caches that keep
only a partial index in memory in the future, we might want to have the ability
to respond with a shrug/I dunno.

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:196: //   to a different key that collides in the
in-memory index).
Hmmm.  I'd rather have an interface that distinguished clearly between
supporting, not-in-cache, and an actual value.  Maybe I'm being silly, but I
could imagine consumers making use of those distinctions.  

How about:

* SetMemoryEntryData() returns false if the feature isn't supported.

* GetMemoryEntryData has an out argument for the value, and return false if the
entry isn't in the cache or the feature isn't supported?

[Maks Orlovich, 2017-06-28 13:54:31]

Thanks Randy, this is exactly the part I wanted feedback on.

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:193: // - If the entry is not in the cache, returns
0.
On 2017/06/27 21:33:56, Randy Smith (Not in Mondays) wrote:
> Definitively isn't in the cache, or isn't in the in-memory index associated
with
> the cache?  I.e. is this case a "Maybe there's an entry with this key--I don't
> know" or "There is no entry with this key in the cache"?

Hmm, good point. It's actually sort of a mixture even for SimpleCache: it can be
the first while we are initializing, then becomes the second.

> The reason I ask is that the ability to be sure that there isn't an entry with
> this key in the cache without checking disk, even in a best effort fashion, is
a
> simple cache feature.  If we want to leave the door open to caches that keep
> only a partial index in memory in the future, we might want to have the
ability
> to respond with a shrug/I dunno.
>

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:196: //   to a different key that collides in the
in-memory index).
On 2017/06/27 21:33:55, Randy Smith (Not in Mondays) wrote:
> Hmmm.  I'd rather have an interface that distinguished clearly between
> supporting, not-in-cache, and an actual value.  Maybe I'm being silly, but I
> could imagine consumers making use of those distinctions.  
> 
> How about:
> 
> * SetMemoryEntryData() returns false if the feature isn't supported.

Not sure how you would use that.

> 
> * GetMemoryEntryData has an out argument for the value, and return false if
the
> entry isn't in the cache or the feature isn't supported?

Hmm, there is also the issue of entry being written by an older version, which
didn't use to have the feature.

[gavinp, 2017-08-04 17:33:14]

lgtm, as long as we clean up the phrasing of the comment mentioned below.

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:193: // - If the entry is not in the cache, returns
0.
On 2017/06/28 13:54:31, Maks Orlovich wrote:
> On 2017/06/27 21:33:56, Randy Smith (Not in Mondays) wrote:
> > Definitively isn't in the cache, or isn't in the in-memory index associated
> with
> > the cache?  I.e. is this case a "Maybe there's an entry with this key--I
don't
> > know" or "There is no entry with this key in the cache"?
> 
> Hmm, good point. It's actually sort of a mixture even for SimpleCache: it can
be
> the first while we are initializing, then becomes the second.
> 

This is subtle; because of course yes we can operate without an index, and the
non-presence of an entry doesn't indicate that we don't have data.

Perhaps move this subtlety out of the context of this comment by rephrasing it
to something more like:

"If this data is not available for this particular key, for any reason, returns
0."

Sound reasonable?


> > The reason I ask is that the ability to be sure that there isn't an entry
with
> > this key in the cache without checking disk, even in a best effort fashion,
is
> a
> > simple cache feature.  If we want to leave the door open to caches that keep
> > only a partial index in memory in the future, we might want to have the
> ability
> > to respond with a shrug/I dunno.
> > 
>

https://codereview.chromium.org/2958033002/diff/1/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/http/mock_http_cache.h#...
net/http/mock_http_cache.h:188: bool support_memory_entry_data_;
This CL could be a good chance to switch to in-class member initialization
rather than the CTOR list. Same for the other class above.

[gavinp, 2017-08-04 17:37:23]

Also, one more thought: why 8 bits. Perhaps it would make sense to grab 12 bits?
Most operating systems allocate files at that granularity, so it's a natural
choice for that reason.

I see this both ways. It's a pain manipulating sub-byte bits.

[Maks Orlovich, 2017-08-04 17:45:28]

Thanks for the feedback, will respond to the rest later, as for this:

On 2017/08/04 17:37:23, gavinp wrote:
> Also, one more thought: why 8 bits. 

Simply because uint8_t is a type, so it naturally expresses how much can be
stored, without going into 
"you have an unsigned int but you can only use 13 bits!" type deal.

[gavinp, 2017-08-04 18:12:04]

On 2017/08/04 17:45:28, Maks Orlovich wrote:
> Thanks for the feedback, will respond to the rest later, as for this:
> 
> On 2017/08/04 17:37:23, gavinp wrote:
> > Also, one more thought: why 8 bits. 
> 
> Simply because uint8_t is a type, so it naturally expresses how much can be
> stored, without going into 
> "you have an unsigned int but you can only use 13 bits!" type deal.

Yeah, I buy that. It's not hard to go to twelve later if we need it.

[Maks Orlovich, 2017-08-23 18:50:16]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-23 18:50:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-08-23 18:52:06]

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/disk_cache/disk_cache.h...
net/disk_cache/disk_cache.h:193: // - If the entry is not in the cache, returns
0.
On 2017/08/04 17:33:14, gavinp wrote:
> On 2017/06/28 13:54:31, Maks Orlovich wrote:
> > On 2017/06/27 21:33:56, Randy Smith (Not in Mondays) wrote:
> > > Definitively isn't in the cache, or isn't in the in-memory index
associated
> > with
> > > the cache?  I.e. is this case a "Maybe there's an entry with this key--I
> don't
> > > know" or "There is no entry with this key in the cache"?
> > 
> > Hmm, good point. It's actually sort of a mixture even for SimpleCache: it
can
> be
> > the first while we are initializing, then becomes the second.
> > 
> 
> This is subtle; because of course yes we can operate without an index, and the
> non-presence of an entry doesn't indicate that we don't have data.
> 
> Perhaps move this subtlety out of the context of this comment by rephrasing it
> to something more like:
> 
> "If this data is not available for this particular key, for any reason,
returns
> 0."
> 
> Sound reasonable?
> 
> 
> > > The reason I ask is that the ability to be sure that there isn't an entry
> with
> > > this key in the cache without checking disk, even in a best effort
fashion,
> is
> > a
> > > simple cache feature.  If we want to leave the door open to caches that
keep
> > > only a partial index in memory in the future, we might want to have the
> > ability
> > > to respond with a shrug/I dunno.
> > > 
> > 

Rephrased, but added "at this time".

https://codereview.chromium.org/2958033002/diff/1/net/http/mock_http_cache.h
File net/http/mock_http_cache.h (right):

https://codereview.chromium.org/2958033002/diff/1/net/http/mock_http_cache.h#...
net/http/mock_http_cache.h:188: bool support_memory_entry_data_;
On 2017/08/04 17:33:14, gavinp wrote:
> This CL could be a good chance to switch to in-class member initialization
> rather than the CTOR list. Same for the other class above.

Not a fan of mixing in very wide-scale cleanups...

[Maks Orlovich, 2017-08-23 19:18:11]

Forwarding comment from other CL to here, where it's better to deal with.

https://codereview.chromium.org/2958033002/diff/40001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/40001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:239: virtual uint8_t GetMemoryEntryData(const
std::string& key);
Suggestion from the impl CL:

pasko 2017/06/29 16:31:49
nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's In-Memory
Data"? Thous would be slightly more readable, I think.

[commit-bot: I haz the power, 2017-08-23 20:06:52]

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

[commit-bot: I haz the power, 2017-08-23 20:06:53]

Dry run: This issue passed the CQ dry run.

[gavinp, 2017-08-25 17:50:17]

still lgtm!

[Maks Orlovich, 2017-08-25 18:34:05]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-25 18:34:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-08-25 18:36:26]

Method name change done

https://codereview.chromium.org/2958033002/diff/40001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2958033002/diff/40001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:239: virtual uint8_t GetMemoryEntryData(const
std::string& key);
On 2017/08/23 19:18:11, Maks Orlovich wrote:
> Suggestion from the impl CL:
> 
> pasko 2017/06/29 16:31:49
> nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's
In-Memory
> Data"? Thous would be slightly more readable, I think.

Done.

[commit-bot: I haz the power, 2017-08-25 20:18:03]

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

[commit-bot: I haz the power, 2017-08-25 20:18:04]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-08-30 13:43:53]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-08-30 13:44:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-30 16:09:00]

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

[commit-bot: I haz the power, 2017-08-30 16:09:02]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-08-30 16:22:15]

The CQ bit was checked by morlovich@chromium.org

[Maks Orlovich, 2017-08-30 16:22:16]

The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org
Link to the patchset: https://codereview.chromium.org/2958033002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-08-30 16:22:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-30 16:28:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1504110135308810,
"parent_rev": "0cb75ffabe51a26775dd5879fda79a565cd2d3b4", "commit_rev":
"1a63f96da596ca12783ef960fa73b8337b5bbbd4"}

[commit-bot: I haz the power, 2017-08-30 16:29:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1504110135308810,
"parent_rev": "a4d6510204bf83d96f7bbcd40ecdbe919ec6d61b", "commit_rev":
"e63bba9024d3397e80e393aed842ba9957385796"}

[commit-bot: I haz the power, 2017-08-30 16:29:19]

Description was changed from

==========
Interface for associating in-memory hint bytes with disk cache entries.
Implementation for it in the mock cache (with off switch)

The intent is to implement it in simple cache, and to use it in
HttpCache::Transaction to avoid opening entries that would not be usable
under present load flags due to caching-hostile headers.

BUG=729679
==========

to

==========
Interface for associating in-memory hint bytes with disk cache entries.
Implementation for it in the mock cache (with off switch)

The intent is to implement it in simple cache, and to use it in
HttpCache::Transaction to avoid opening entries that would not be usable
under present load flags due to caching-hostile headers.

BUG=729679

Review-Url: https://codereview.chromium.org/2958033002
Cr-Commit-Position: refs/heads/master@{#498491}
Committed:
https://chromium.googlesource.com/chromium/src/+/e63bba9024d3397e80e393aed842...
==========

[commit-bot: I haz the power, 2017-08-30 16:29:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e63bba9024d3397e80e393aed842...