RFC: use some in-memory state in SimpleCache to quickly cache-miss some CantConditionalize cases

Sketch permitting clients to store upto a byte of info with an entry in in-memory index of SimpleCache, and to permit using a callback to use that information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with zero freshness and not enough info to refresh, when load flags don't require us to use them.

(This is crude, and is missing some test changes. Also missing needed UMA changes. It would also at the very least be sensibly splittable into 2 CLs for actual review)

BUG=729679

[Maks Orlovich, 2017-06-05 19:07:53]

Description was changed from

==========
Sketch of oracle byte support in SimpleCache, minus persistence.

BUG=
==========

to

==========
Sketch of oracle byte support in SimpleCache

BUG=
==========

[Maks Orlovich, 2017-06-05 19:11:15]

Description was changed from

==========
Sketch of oracle byte support in SimpleCache

BUG=
==========

to

==========
Sketch of oracle byte support in SimpleCache

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

[Maks Orlovich, 2017-06-06 15:34:58]

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

[commit-bot: I haz the power, 2017-06-06 15:35:20]

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-06 15:48:49]

Description was changed from

==========
Sketch of oracle byte support in SimpleCache

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

to

==========
Sketch permitting clients to store upto a byte of info with an entry in
in-memory index of SimpleCache, and to permit using a callback to use that
information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with
zero freshness and not enough info to refresh, when load flags don't require us
to use them.

(This is crude, and is missing some SimpleCache test changes, including updates
due to entry disk sizes getting rounded, and tests for new functionality. It
would also at the very least be sensibly splittable into 2 CLs for actual
review)

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

[Maks Orlovich, 2017-06-06 16:04:53]

Description was changed from

==========
Sketch permitting clients to store upto a byte of info with an entry in
in-memory index of SimpleCache, and to permit using a callback to use that
information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with
zero freshness and not enough info to refresh, when load flags don't require us
to use them.

(This is crude, and is missing some SimpleCache test changes, including updates
due to entry disk sizes getting rounded, and tests for new functionality. It
would also at the very least be sensibly splittable into 2 CLs for actual
review)

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

to

==========
Sketch permitting clients to store upto a byte of info with an entry in
in-memory index of SimpleCache, and to permit using a callback to use that
information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with
zero freshness and not enough info to refresh, when load flags don't require us
to use them.

(This is crude, and is missing some SimpleCache test changes, including updates
due to entry disk sizes getting rounded, and tests for new functionality. Also
missing needed UMA changes. It would also at the very least be sensibly
splittable into 2 CLs for actual review)

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

[commit-bot: I haz the power, 2017-06-06 16:05:36]

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

[commit-bot: I haz the power, 2017-06-06 16:05:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Maks Orlovich, 2017-06-06 16:52:55]

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

[commit-bot: I haz the power, 2017-06-06 16:53: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-06 17:41:14]

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

[commit-bot: I haz the power, 2017-06-06 17:41:33]

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-06-06 19:16:27]

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

[commit-bot: I haz the power, 2017-06-06 19:16:28]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-06-06 19:21:40]

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

[commit-bot: I haz the power, 2017-06-06 19:22:04]

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-06 19:26:23]

Description was changed from

==========
Sketch permitting clients to store upto a byte of info with an entry in
in-memory index of SimpleCache, and to permit using a callback to use that
information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with
zero freshness and not enough info to refresh, when load flags don't require us
to use them.

(This is crude, and is missing some SimpleCache test changes, including updates
due to entry disk sizes getting rounded, and tests for new functionality. Also
missing needed UMA changes. It would also at the very least be sensibly
splittable into 2 CLs for actual review)

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

to

==========
Sketch permitting clients to store upto a byte of info with an entry in
in-memory index of SimpleCache, and to permit using a callback to use that
information to veto a hit. 

Use that inside HttpCache::Transaction to quickly reject cached entries with
zero freshness and not enough info to refresh, when load flags don't require us
to use them.

(This is crude, and is missing some test changes. Also missing needed UMA
changes. It would also at the very least be sensibly splittable into 2 CLs for
actual review)

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

[Maks Orlovich, 2017-06-06 19:30:08]

morlovich@chromium.org changed reviewers:
+ jkarlin@chromium.org, rdsmith@chromium.org, shivanisha@chromium.org

[Maks Orlovich, 2017-06-06 19:30:09]

So this is more of a request for a design review than for an actual code review;
this isn't in good enough shape for later, at any rate. If you prefer a proper
design doc instead, I can do one, though the "storing more things in index"
section in
https://docs.google.com/a/google.com/document/d/1BHKA2PRAD43SUMtMb8ajBjTcYB6O...
is a sort of high-level one. 

The changes that are of general interest are to net/disk_cache/disk_cache.h and
net/http/ (the SimpleCache changes are less interesting for this audience, I
feel). The pain point here is basically that this is trying to summarize the
effect of RequiresValidation() and !ResponseConditionalizable() in a few bits. 
(Also I have no clue on what to do with UMA reporting, since CantConditionalize
stuff currently goes through a hit...)

[commit-bot: I haz the power, 2017-06-06 19:56:28]

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

[commit-bot: I haz the power, 2017-06-06 19:56:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Randy Smith (Not in Mondays), 2017-06-07 19:55:38]

On 2017/06/06 19:30:09, Maks Orlovich wrote:
> So this is more of a request for a design review than for an actual code
review;
> this isn't in good enough shape for later, at any rate. If you prefer a proper
> design doc instead, I can do one, though the "storing more things in index"
> section in
>
https://docs.google.com/a/google.com/document/d/1BHKA2PRAD43SUMtMb8ajBjTcYB6O...
> is a sort of high-level one. 
> 
> The changes that are of general interest are to net/disk_cache/disk_cache.h
and
> net/http/ (the SimpleCache changes are less interesting for this audience, I
> feel). The pain point here is basically that this is trying to summarize the
> effect of RequiresValidation() and !ResponseConditionalizable() in a few bits.

> (Also I have no clue on what to do with UMA reporting, since
CantConditionalize
> stuff currently goes through a hit...)

A couple of thoughts:
* The basic concept looks sound to me.
* I'm wincing at the API cleanliness of the change to the disk cache interface,
though hopefully this turns into just nits.  We can work that out in the "real"
CL, but my thoughts are vaguely in the direction of: Wanting to have only one
OpenEntry() interface, not wanting to refer to this as "Oracle" bytes--maybe
frame the interface as maybe "cached data" or "cached byte" indicating that it's
more rapidly accessible, wanting the callback to be about aborting the open in
response to the cached data, and making the result of a default constructed
callback or the lack of cached data being not to abort.  
* I started and then semi-abandoned a possibly relevant CL that combined
RequiresValidation() and ConditionalizeRequest(); you're welcome to grab stuff
from it if it looks useful.  (Not saying it is, I was just struggling around
code patterns around RequiresValidation & ConditionalizeRequest() and if you are
as well it might be worth looking at).  See
https://codereview.chromium.org/2854433002/.  But looking more closely at what
you're doing, I don't think it's useful.  Never mind (unless you're curious).

[jkarlin, 2017-06-07 20:01:54]

Awesome that you're looking at this case. Thoughts below.

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

https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:127: virtual int OpenEntryWithOracleByte(const
std::string& key,
I'd rather we have a more general purpose byte in the index that can be used by
any customer. OracleJudgement and OpenEntryWithOracleByte forces a certain
behavior for the byte.

How about instead:

virtual uint8_t GetMemoryEntryData(const std::string& key);
virtual void SetMemoryEntryData(const std::string& key, uint8_t data);

Then the HttpCache::Transaction can call that and make its own decision before
the call to Open.

[jkarlin, 2017-06-07 20:04:45]

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

https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache.h:127: virtual int OpenEntryWithOracleByte(const
std::string& key,
On 2017/06/07 20:01:54, jkarlin wrote:
> I'd rather we have a more general purpose byte in the index that can be used
by
> any customer. OracleJudgement and OpenEntryWithOracleByte forces a certain
> behavior for the byte.
> 
> How about instead:
> 
> virtual uint8_t GetMemoryEntryData(const std::string& key);
> virtual void SetMemoryEntryData(const std::string& key, uint8_t data);
> 
> Then the HttpCache::Transaction can call that and make its own decision before
> the call to Open.

Ah, this will have to be an async call won't it because the index might be still
loading. That's still okay imo.

[morlovich, 2017-06-07 20:28:24]

> How about instead:
> 
> virtual uint8_t GetMemoryEntryData(const std::string& key);
> virtual void SetMemoryEntryData(const std::string& key, uint8_t data);

So a problem with this is that we can't tell if the byte is associated with
|key|, or something that 
has the same hash as |key|, as the in-memory index only stores the hash. That's
OK in case of open since we end up 
dooming the entry on key collision anyway, and that's basically the only thing
one can do with the proposed API.

In other cases? Seems dangerous.

... This should probably be written down somewhere, regardless, shouldn't it?

>Ah, this will have to be an async call won't it because the index might be
still
> loading. That's still okay imo.

Can be sync if it's permitted to return 0/some other fallback value... which it
has to be able to do in case of 
index recovery (or old index version) anyway, or if the backend doesn't support
it. (I don't think I could
implement it in block cache, and it might be pointless for memory one).

[jkarlin, 2017-06-07 20:49:37]

On 2017/06/07 20:28:24, morlovich wrote:
> > How about instead:
> > 
> > virtual uint8_t GetMemoryEntryData(const std::string& key);
> > virtual void SetMemoryEntryData(const std::string& key, uint8_t data);
> 
> So a problem with this is that we can't tell if the byte is associated with
> |key|, or something that 
> has the same hash as |key|, as the in-memory index only stores the hash.
That's
> OK in case of open since we end up 
> dooming the entry on key collision anyway, and that's basically the only thing
> one can do with the proposed API.
> 
> In other cases? Seems dangerous.
> 
> ... This should probably be written down somewhere, regardless, shouldn't it?

Yet another reason for having the key in the index (CacheStorage would like to
have that). Let's see, with a 64 bit hash we'd expect the chance of collision to
be 1 in 10 million with 2 million entries. I think it's safe to put a warning in
the header that collisions could happen and not to use the data for anything
security related.

> 
> >Ah, this will have to be an async call won't it because the index might be
> still
> > loading. That's still okay imo.
> 
> Can be sync if it's permitted to return 0/some other fallback value... which
it
> has to be able to do in case of 
> index recovery (or old index version) anyway, or if the backend doesn't
support
> it. (I don't think I could
> implement it in block cache, and it might be pointless for memory one).

True.. in which case sync sounds good.

[jkarlin, 2017-06-08 14:44:07]

https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:92: return entry_size_ & 0xFFFFFF00;
I'd rather add a byte (or a uint32_t even) to the index than hide a byte within
the existing struct. Hiding just complicates the code's logic and makes
GetEntrySize less precise.

[Maks Orlovich, 2017-06-09 14:49:24]

On 2017/06/07 20:49:37, jkarlin wrote:
> Yet another reason for having the key in the index (CacheStorage would like to
> have that). 

That's probably too wasteful for main caches. An estimate from my work machine's
cache 
(not likely to be representative, and it makes std::string objects 1 byte big,
but heck,
for magnitude):

$ out/Default/cachetool ~/.cache/google-chrome-beta/Default/Cache simple
list_keys > /tmp/keys.txt
$ wc -c /tmp/keys.txt 
3095905 /tmp/keys.txt

Having flexibility in what the memory index is, and maybe a different
implementation for APP_CACHE might
be reasonable, though.

> Let's see, with a 64 bit hash we'd expect the chance of collision to
> be 1 in 10 million with 2 million entries. I think it's safe to put a warning
in
> the header that collisions could happen and not to use the data for anything
> security related.

Hmm, you have me almost convinced. Not having the messy presence of 2 OpenEntry
methods,
no callbacks, and simple naming (rather than not-really-correctly-used theory
term) is definitely
enticing, but I am a little worried by the borderline-impossible to test caveat.

Do any of the other reviewers have a take?


On 2017/06/08 14:44:07, jkarlin wrote:
>
https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:92: return entry_size_ & 0xFFFFFF00;
> I'd rather add a byte (or a uint32_t even) to the index than hide a byte
within
> the existing struct. Hiding just complicates the code's logic and makes
> GetEntrySize less precise.

I could actually argue it makes it more precise, since the method is about disk
usage, and not too many 
file systems can allocate things with byte precision. More pressingly, though,
this sort of rounding does
make a given cache size setting likely store a bit less, which is more important
than being pedantic.

WRT to readability, bitfields might help some, or they might not. If not
bitfields, at very least entry_size_
does need renaming if the implementation remains the same.

I don't have a good sense of how worried we should be about using up X KiB,
though --- that goes for the LRU2 
CL as well. Those things have a way of adding up.

[jkarlin, 2017-06-09 15:34:26]

On 2017/06/09 14:49:24, Maks Orlovich wrote:
> On 2017/06/07 20:49:37, jkarlin wrote:
> > Yet another reason for having the key in the index (CacheStorage would like
to
> > have that). 
> 
> That's probably too wasteful for main caches. An estimate from my work
machine's
> cache 
> (not likely to be representative, and it makes std::string objects 1 byte big,
> but heck,
> for magnitude):
> 
> $ out/Default/cachetool ~/.cache/google-chrome-beta/Default/Cache simple
> list_keys > /tmp/keys.txt
> $ wc -c /tmp/keys.txt 
> 3095905 /tmp/keys.txt

Ouch :(

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

On 2017/06/09 14:49:24, Maks Orlovich wrote:
> On 2017/06/07 20:49:37, jkarlin wrote:
> > Yet another reason for having the key in the index (CacheStorage would like
to
> > have that). 
> 
> That's probably too wasteful for main caches. An estimate from my work
machine's
> cache 
> (not likely to be representative, and it makes std::string objects 1 byte big,
> but heck,
> for magnitude):
> 
> $ out/Default/cachetool ~/.cache/google-chrome-beta/Default/Cache simple
> list_keys > /tmp/keys.txt
> $ wc -c /tmp/keys.txt 
> 3095905 /tmp/keys.txt
> 
> Having flexibility in what the memory index is, and maybe a different
> implementation for APP_CACHE might
> be reasonable, though.
> 
> > Let's see, with a 64 bit hash we'd expect the chance of collision to
> > be 1 in 10 million with 2 million entries. I think it's safe to put a
warning
> in
> > the header that collisions could happen and not to use the data for anything
> > security related.
> 
> Hmm, you have me almost convinced. Not having the messy presence of 2
OpenEntry
> methods,
> no callbacks, and simple naming (rather than not-really-correctly-used theory
> term) is definitely
> enticing, but I am a little worried by the borderline-impossible to test
caveat.
> 
> Do any of the other reviewers have a take?

So I'm certain what aspects of the proposal you're most focussed on.  I like the
generality of Josh's proposal--oracle byte seems too specific for me.  I think
it's reasonable to document that this is likely-but-not-certain to apply to the
given key, and hence shouldn't be relied on for functionality, only performance.
 I'm a bit concerned about the implication that there's only one consumer of the
backend (in that all consumers share the same byte, so if you have more than one
you have conceptual collision over what it's used for) and I'm not sure how to
document our way out of that.  WRT testing ... you could test based on the known
implementation (whitebox) on a per-backend basis, at which point the behavior is
precisely defined (if there's a key collision, you lose, otherwise you don't--at
least I think that's what the SimpleCache would provide).  But something that's
targeted at performance not functionality should probably have some performance
testing, which would require a wide variety of keys and would probably be pretty
complex, but still, I'd think could work.

So overall: I'm in favor (which was probably clear from my comments above), but
a bit worried about the (new?) implication in the interface that there's just
one consumer for the disk cache.

> 
> 
> On 2017/06/08 14:44:07, jkarlin wrote:
> >
>
https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
> > File net/disk_cache/simple/simple_index.cc (right):
> > 
> >
>
https://codereview.chromium.org/2922973003/diff/140001/net/disk_cache/simple/...
> > net/disk_cache/simple/simple_index.cc:92: return entry_size_ & 0xFFFFFF00;
> > I'd rather add a byte (or a uint32_t even) to the index than hide a byte
> within
> > the existing struct. Hiding just complicates the code's logic and makes
> > GetEntrySize less precise.
> 
> I could actually argue it makes it more precise, since the method is about
disk
> usage, and not too many 
> file systems can allocate things with byte precision. More pressingly, though,
> this sort of rounding does
> make a given cache size setting likely store a bit less, which is more
important
> than being pedantic.
> 
> WRT to readability, bitfields might help some, or they might not. If not
> bitfields, at very least entry_size_
> does need renaming if the implementation remains the same.
> 
> I don't have a good sense of how worried we should be about using up X KiB,
> though --- that goes for the LRU2 
> CL as well. Those things have a way of adding up.

[jkarlin, 2017-06-09 18:03:35]

>  I'm a bit concerned about the implication that there's only one consumer of
the
> backend (in that all consumers share the same byte, so if you have more than
one
> you have conceptual collision over what it's used for) and I'm not sure how to
> document our way out of that.  

Are you referring to multiple consumers of the DISK_CACHE? I think all roads
lead to the HttpCache to talk to that backend. The others
(CacheStorage/AppCache/etc.) don't share the same backend instance, they have
their own and can use the byte in their own ways.

[Randy Smith (Not in Mondays), 2017-06-09 18:07:11]

On 2017/06/09 18:03:35, jkarlin wrote:
> >  I'm a bit concerned about the implication that there's only one consumer of
> the
> > backend (in that all consumers share the same byte, so if you have more than
> one
> > you have conceptual collision over what it's used for) and I'm not sure how
to
> > document our way out of that.  
> 
> Are you referring to multiple consumers of the DISK_CACHE? I think all roads
> lead to the HttpCache to talk to that backend. The others
> (CacheStorage/AppCache/etc.) don't share the same backend instance, they have
> their own and can use the byte in their own ways.

I'm raising an abstract Truth&Beauty concern as I'm not sure there are other
aspects of the interface that wouldn't work well for multiple consumers.  In our
concrete case I don't think it's much of a problem, but I did think there were
some corner cases of using the backend to store data "around"
HttpCache::Transaction but to the same instance of the backend (for QUIC/SPDY
stuff?).  But I can't find an example in codesearch, so presumably I'm either
mis-remembering or that's been fixed.

[Maks Orlovich, 2017-06-09 20:03:34]

morlovich@chromium.org changed reviewers:
+ pasko@chromium.org
- shivanisha@chromium.org

[Maks Orlovich, 2017-06-12 14:29:51]

Hmm, a different concern: looking through higher-level failures, one is:
IN_PROC_BROWSER_TEST_F(ErrorPageTest, StaleCacheStatus) {
Which points out the error page can offer to open a stale entry in case of
network error;
and obviously turning it into a miss breaks that.

Not sure how important that is, obviously letting users open from cache in such
a case
is extremely helpful, but loading pages quicker is also nice.

[Randy Smith (Not in Mondays), 2017-06-12 15:15:42]

On 2017/06/12 14:29:51, Maks Orlovich wrote:
> Hmm, a different concern: looking through higher-level failures, one is:
> IN_PROC_BROWSER_TEST_F(ErrorPageTest, StaleCacheStatus) {
> Which points out the error page can offer to open a stale entry in case of
> network error;
> and obviously turning it into a miss breaks that.
> 
> Not sure how important that is, obviously letting users open from cache in
such
> a case
> is extremely helpful, but loading pages quicker is also nice.

FWIW, that functionality isn't being used anymore, though there plumbing is
extensive enough that it would be some work to remove it.

[Maks Orlovich, 2017-06-12 19:39:52]

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

[commit-bot: I haz the power, 2017-06-12 19:40:36]

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-12 20:08:38]

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

[commit-bot: I haz the power, 2017-06-12 20:09:07]

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-06-12 20:39:47]

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

[commit-bot: I haz the power, 2017-06-12 20:39:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pasko, 2017-06-13 14:30:05]

(back from vacation)

the high level design looks reasonable, and I'm excited about the potential
improvement! Thank you for pushing in this direction!

There are a few details to figure out, as mentioned already, from API
cleanliness to the cache transaction correctness, which it seems would be more
productive if you do it mostly with jkarlin@/rdsmith@.

Let me know once/if you want me to have a final pass over the changes on the
simplecache side.

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:85: // Storage size in 256-byte blocks
4K blocks, if rounded upwards would probably be even more representative on how
much this occupies on the filesystem. WDYT?

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:87: uint8_t memory_entry_data_ : 8;
it would be cleaner to reserve one bit that says we did not loose the data
because of the issues with the index file. It will help with metrics and making
sure that bits set to 0 are meaningful sometimes.

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.h#...
net/http/http_cache.h:75: OBH_UNUSED_SINCE_PREFETCH = 4
it's nice to have it here in general, but probably belongs to a separate change,
as it is an orthogonal optimization (and somewhat orthogonal to
cantconditionalize).

[Maks Orlovich, 2017-06-13 14:55:29]

> I'm raising an abstract Truth&Beauty concern as I'm not sure there are other
> aspects of the interface that wouldn't work well for multiple consumers.  In
our
> concrete case I don't think it's much of a problem, but I did think there were
> some corner cases of using the backend to store data "around"
> HttpCache::Transaction but to the same instance of the backend (for QUIC/SPDY
> stuff?).  But I can't find an example in codesearch, so presumably I'm either
> mis-remembering or that's been fixed.

I guess I don't see how this is different from everything else. If more than one
consumers wants to use, say, stream 0 for the same entry, they have to come up
with some
protocol to not stomp over each other, too. Here they could in principle
partition up
the bits if it's needed. 

> FWIW, that functionality isn't being used anymore, though there plumbing is
> extensive enough that it would be some work to remove it.

Ah, I see now that the tests actually add some switches to get it enabled. That
makes it at least 
philosophically an easier question, though I am pretty ignorant of all the
practicalities.

[Maks Orlovich, 2017-06-13 14:58:48]

Might be different enough to be worth another look; I am thinking maybe
splitting off just the disk_cache API change + the mock implementation of them
may be a good step for moving it forward?

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:85: // Storage size in 256-byte blocks
On 2017/06/13 14:30:05, pasko wrote:
> 4K blocks, if rounded upwards would probably be even more representative on
how
> much this occupies on the filesystem. WDYT?

Hmm, maybe. Plus: it would also prevent things from changing again if we steal a
bit somewhere, and it's somewhat less arbitrary. 

OTOH, it would make cache utilization (even) less. 

@jkarlin: you know things about how cache size affects things, don't you?

https://codereview.chromium.org/2922973003/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:87: uint8_t memory_entry_data_ : 8;
On 2017/06/13 14:30:05, pasko wrote:
> it would be cleaner to reserve one bit that says we did not loose the data
> because of the issues with the index file. It will help with metrics and
making
> sure that bits set to 0 are meaningful sometimes.

Hmm, maybe with a bool GetMemoryEntryData(uint64_t entry_hash, uint8_t* out)
const ? Could be, but I am worried about wasting bits. Really, 8 bits maybe too
much, but it makes for at least somewhat natural API.

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.h#...
net/http/http_cache.h:75: OBH_UNUSED_SINCE_PREFETCH = 4
On 2017/06/13 14:30:05, pasko wrote:
> it's nice to have it here in general, but probably belongs to a separate
change,
> as it is an orthogonal optimization (and somewhat orthogonal to
> cantconditionalize).

We have to deal with it, since it affects
HttpCache::Transaction::RequiresValidation:

  if (response_.unused_since_prefetch &&
      !(effective_load_flags_ & LOAD_PREFETCH) &&
      response_.headers->GetCurrentAge(
          response_.request_time, response_.response_time,
          cache_->clock_->Now()) < TimeDelta::FromMinutes(kPrefetchReuseMins)) {
    // The first use of a resource after prefetch within a short window skips
    // validation.
    return false;
  }

e.g. things that have unused_since_prefetch set have to be opened since if
they're recent they may be used regardless caching headers. Could probably make
CanRejectBasedOnMemoryEntryData pay attention to 
(effective_load_flags_ & LOAD_PREFETCH) though.

[Randy Smith (Not in Mondays), 2017-06-13 17:52:21]

I like the disk cache interface as written.  I'm inclined to defer to others on
the simple cache implementation and the usage in HC::T, but am willing to do
either of those reviews if you can't find someone better.  But I think the basic
outline of this plan is fine for turning this into a real, ready-to-land CL. 
(Obviously, get others opinions on that key question before you do so :-}.)

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.cc...
net/http/http_cache.cc:761: return net::ERR_FAILED;
Won't this result in failing the consuming HttpCache::Transacation/URLRequest? 
Don't we instead want to re-create the entry based on the
now-required-to-be-sent network request?

[Maks Orlovich, 2017-06-13 18:15:08]

(Will also reply to the main conversation there separately.. doesn't seem like I
can do it in one e-mail?)

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2922973003/diff/180001/net/http/http_cache.cc...
net/http/http_cache.cc:761: return net::ERR_FAILED;
On 2017/06/13 17:52:21, Randy Smith (Not in Mondays) wrote:
> Won't this result in failing the consuming HttpCache::Transacation/URLRequest?

> Don't we instead want to re-create the entry based on the
> now-required-to-be-sent network request?

It's the same status a miss on OpenEntry would return in the backend:

https://cs.chromium.org/chromium/src/net/disk_cache/simple/simple_entry_impl....
called from:
https://cs.chromium.org/chromium/src/net/disk_cache/simple/simple_backend_imp...

(And dooming the entry here is what prepares for the eventual CreateEntry to
succeed). 

There is an alternative approach I am sketching right now, though, and that's
basically to try to actually handle it in the state machine rather than treating
it as a miss. Advantages:
1) Way better at actually UMAing things.
2) Sets was_cached correctly so I think the higher-level stuff would work.
(The test failure I am getting are just reduction in open_count from cache, but
of course I only ran net_unittests)

Disadvantages:
1) It calls DoomEntry later.
2) Trying to fast-forward through the state machine is messy, and we are not
*really* in the same state as it would be, given the entry isn't actually read.
In general the whole thing ends up much harder to reason about, and I am not
even sure how to implement it cleanly (I ended up changing next_state_ directly
just to try it out).

I'll probably upload the ultra-dirty patch just so I could trybot it, but it's
likely I'll abandon that direction unless I can figure something out to refine
it.

[Maks Orlovich, 2017-06-13 18:25:21]

On 2017/06/13 17:52:21, Randy Smith (Not in Mondays) wrote:
> I like the disk cache interface as written.  I'm inclined to defer to others
on
> the simple cache implementation and the usage in HC::T, but am willing to do
> either of those reviews if you can't find someone better. 

I figure I can ask pasko@ and/or Gavin (depending on how much he sync up) for
the backend,
but I am not sure there are better candidates for HC::T than you and Josh?

> But I think the basic outline of this plan is fine for turning this into a
real, ready-to-land CL. 
> (Obviously, get others opinions on that key question before you do so :-}.)

Thanks.

[Maks Orlovich, 2017-06-13 18:33:54]

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

[commit-bot: I haz the power, 2017-06-13 18:34:41]

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-06-13 19:05:38]

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

[commit-bot: I haz the power, 2017-06-13 19:05:40]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Maks Orlovich, 2017-06-13 19:44:49]

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

[commit-bot: I haz the power, 2017-06-13 19:45:42]

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-06-13 20:18:14]

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

[commit-bot: I haz the power, 2017-06-13 20:18:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Maks Orlovich, 2017-06-13 21:00:01]

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

[commit-bot: I haz the power, 2017-06-13 21:01: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-06-13 22:02:44]

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

[commit-bot: I haz the power, 2017-06-13 22:02:45]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Maks Orlovich, 2017-06-14 15:57:39]

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

[commit-bot: I haz the power, 2017-06-14 15:58:05]

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-06-14 16:00:35]

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

[commit-bot: I haz the power, 2017-06-14 16:00:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Maks Orlovich, 2017-06-14 16:35:41]

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

[commit-bot: I haz the power, 2017-06-14 16:35:53]

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-14 17:26:27]

morlovich@chromium.org changed reviewers:
+ gavinp@chromium.org

[commit-bot: I haz the power, 2017-06-14 18:13:12]

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

[commit-bot: I haz the power, 2017-06-14 18:13:13]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-06-14 19:59:30]

https://codereview.chromium.org/2922973003/diff/280001/net/http/http_cache_tr...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2922973003/diff/280001/net/http/http_cache_tr...
net/http/http_cache_transaction.cc:2560: next_state_ = STATE_SEND_REQUEST;
So I just realized that this implementation is bogus, since we want to go
through CREATE_ENTRY and the like. The previous version didn't have this issue,
though.

[Maks Orlovich, 2017-06-15 14:47:28]

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

[commit-bot: I haz the power, 2017-06-15 14:47:42]

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-06-15 16:07:19]

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

[commit-bot: I haz the power, 2017-06-15 16:07:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

On 2017/06/13 18:25:21, Maks Orlovich wrote:
> On 2017/06/13 17:52:21, Randy Smith (Not in Mondays) wrote:
> > I like the disk cache interface as written.  I'm inclined to defer to others
> on
> > the simple cache implementation and the usage in HC::T, but am willing to do
> > either of those reviews if you can't find someone better. 
> 
> I figure I can ask pasko@ and/or Gavin (depending on how much he sync up) for
> the backend,
> but I am not sure there are better candidates for HC::T than you and Josh?

Shivani's probably the expert at this point, but yeah Josh and I should be at
least close seconds.
> 
> > But I think the basic outline of this plan is fine for turning this into a
> real, ready-to-land CL. 
> > (Obviously, get others opinions on that key question before you do so :-}.)
> 
> Thanks.

[gavinp, 2017-06-26 20:18:11]

Generally looks good to me; previous comments captured most of what I think.

I imagine we'll want UMA so we can see how significant each of these is?

Pasko mentioned having a bit to indicate if the data is present. I don't think
that's needed quite yet; the semantics so far work for 0 values. If we add
something more nuanced, I guess we'll write the more complex coding class then?

https://codereview.chromium.org/2922973003/diff/300001/net/http/mock_http_cac...
File net/http/mock_http_cache.h (right):

https://codereview.chromium.org/2922973003/diff/300001/net/http/mock_http_cac...
net/http/mock_http_cache.h:67: uint8_t memory_entry_data() const { return
memory_entry_data_; }
Is this implemented here to make sure that http_cache_transaction unit tests
test this path?

Should we provide an implementation that mimics the blockfile cache, and always
returns 0?

[pasko, 2017-06-27 09:53:51]

On 2017/06/26 20:18:11, gavinp OOO until May wrote:
> Generally looks good to me; previous comments captured most of what I think.
> 
> I imagine we'll want UMA so we can see how significant each of these is?
> 
> Pasko mentioned having a bit to indicate if the data is present. I don't think
> that's needed quite yet; the semantics so far work for 0 values. If we add
> something more nuanced, I guess we'll write the more complex coding class
then?

I asked for it because I think this way it is going to be more readable and less
error prone plus more metrics-happy-wrt-lost-index. No doubt 0 values work for
now. Maybe I am exaggerating the benefits of this "simplification". Up to you
guys.

[gavinp, 2017-08-04 18:52:34]

@morlovich: What's happening with this issue? I've seen two CLs from here
already, and I guess there's one or two more to go?

Is there anything to do advancing this sketch, or is it done now? Let me know if
I can contribute anything.

[Maks Orlovich, 2017-08-04 18:56:46]

On 2017/08/04 18:52:34, gavinp wrote:
> @morlovich: What's happening with this issue? I've seen two CLs from here
> already, and I guess there's one or two more to go?
> 
> Is there anything to do advancing this sketch, or is it done now? Let me know
if
> I can contribute anything.

This one is just the proof of concept for the two pieces you individually
reviewed, plus a not-yet-created future CL to actually use that infrastructure 
in HttpCache::Transaction. A full review of this would likely be a waste of time
(since large parts are cruder versions of what you already reviewed), but 
there may opportunities to save me from future pain in looking at the
http_cache* changes only.