Race Create vs SendRequest on cache misses.

Race Create vs SendRequest on cache misses.

By allowing CreateEntry to return instantly with a proxy object, the disk cache is significantly sped up in the miss case.

[gavinp, 2013-01-08 21:16:21]

Ricardo,

I know this isn't ready for landing, but I'd appreciate you taking a peek at it.
The big question I have is about testing; if you force the experiment on for
unit tests, then almost all of the net_unittests pass. The only ones that fail
are making specific assertions about the event log etc... assuming the state
machine followed a non-racing create path.

How do you think it's best to approach testing? Create a few specific unit tests
that use the race? Run each test twice, racing and not racing? I tend to favour
the latter, and marking out the tests that have to be different when they're
racing.

[rvargas (doing something else), 2013-01-09 20:01:51]

I don't really think that testing is the more concerning part here.

This CL logically splits the state machine for the duration of one state and
bails out after that point if something unexpected (or hard to handle) happen on
either branch. Again, this is not something that we do (bailing out), or not at
least with predictable conditions.

I still think that this is the wrong place to write a solution for a slow disk
cache. We've waited many months for getting a change at the right layer and I
don't see the reason to write a quick hack at this point.

[pasko-google - do not use, 2013-01-09 22:46:14]

On 2013/01/09 20:01:51, rvargas wrote:
> I still think that this is the wrong place to write a solution for a slow disk
> cache. We've waited many months for getting a change at the right layer and I
> don't see the reason to write a quick hack at this point.

Ricardo, what do you mean by the change in the right layer? Is it under review
now or committed under a flag?

[rvargas (doing something else), 2013-01-10 00:21:02]

On 2013/01/09 22:46:14, pasko wrote:
> On 2013/01/09 20:01:51, rvargas wrote:
> > I still think that this is the wrong place to write a solution for a slow
disk
> > cache. We've waited many months for getting a change at the right layer and
I
> > don't see the reason to write a quick hack at this point.
> 
> Ricardo, what do you mean by the change in the right layer? Is it under review
> now or committed under a flag?

Let's say that there is a disk cache implementation that performs CreateEntry
operations 99% of the time in less than 1 ms, but can take up to 2 seconds from
time to time. Why does the http cache has to implement a work around for that
behavior?

The performance characteristics of the current CreateEntry disk cache code are
well known since it was implemented (in fact, since the design phase). We now
feel that this is not the right tradeoff (also, nothing new here), and we've
been delaying writing a solution at the right layer (the disk cache code). So
no, I'm not saying that there's a CL under review, just that this is the wrong
fix.

[pasko-google - do not use, 2013-01-10 14:05:44]

On 2013/01/10 00:21:02, rvargas wrote:
> On 2013/01/09 22:46:14, pasko wrote:
> > On 2013/01/09 20:01:51, rvargas wrote:
> > > I still think that this is the wrong place to write a solution for a slow
> disk
> > > cache. We've waited many months for getting a change at the right layer
and
> I
> > > don't see the reason to write a quick hack at this point.
> > 
> > Ricardo, what do you mean by the change in the right layer? Is it under
review
> > now or committed under a flag?
> 
> Let's say that there is a disk cache implementation that performs CreateEntry
> operations 99% of the time in less than 1 ms, but can take up to 2 seconds
from
> time to time. Why does the http cache has to implement a work around for that
> behavior?

I understand your concern. If there are higher priority issues, we should
definitely work on those, but the issue itself is not quite innocent.

If we assume a typical amount of resources on a page to be 100, and cache misses
account for 50% of resources, my back of the envelope calculations on top of
current UMA stats on desktop suggest that about 0.04% of pages will have latency
of 2s.

Now let's assume that N=20 members of the Chrome team independently apply this
argument of not dealing with a 2s unreasonable latency in their subsystem for
0.04% pages. Then 8% of the pages would have a problem. With N=40 28% of pages
have a problem.

Scary.

[pasko-google - do not use, 2013-01-10 16:14:27]

On 2013/01/10 14:05:44, pasko wrote:
> current UMA stats on desktop suggest that about 0.04% of pages will have
latency
> of 2s.
> 
> Now let's assume that N=20 members of the Chrome team independently apply this
> argument of not dealing with a 2s unreasonable latency in their subsystem for
> 0.04% pages. Then 8% of the pages would have a problem. With N=40 28% of pages
> have a problem.

I made a mistake in calculations. For N=2 the 2s latency should happen in 0.8%
pages, for N=40 it is 1.6%. The point is that slowdowns "add up". Also they make
benchmarking harder by increasing variability.

[rvargas (doing something else), 2013-01-10 19:09:12]

To close the loop, I'm not saying we should not care about this issue, I'm
saying that this is not the right layer to fix it.

(btw, I think your calculations assume a uniform distribution of "delays", and I
don't think that's the case)

[gavinp, 2013-01-23 15:08:30]

Ricardo,

Can you take a brief look at this architecturally?

- Gavin

[rvargas (doing something else), 2013-01-23 22:04:09]

https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_cr...
File net/disk_cache/pending_create_entry.cc (right):

https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_cr...
net/disk_cache/pending_create_entry.cc:18: // to true if it receives any
operation we cannot proxy on the new entry.
This object should handle the case and keep going on without failing (or at
least do a reasonable attempt to do so).

https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_cr...
File net/disk_cache/pending_create_entry.h (right):

https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_cr...
net/disk_cache/pending_create_entry.h:21: // has been created, operations may
return net::ERR_CACHE_CREATE_RACE_FAILED.
If we return an "entry" synchronously, the caller should be able to use it
immediately no matter what. Returning a delayed error because some work has not
been done is not nice.

Again, I don't think we should be "racing" the operations this way. It changes
behavior from what should be currently a rare event (a real error after the
transaction has started) to a common event (whoops, I didn't finish in time).

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

https://codereview.chromium.org/11787007/diff/16001/net/http/http_cache.cc#ne...
net/http/http_cache.cc:716: int rv =
disk_cache::PendingCreateEntry(disk_cache_.get(), key, &new_entry);
why is this not calling CreateEntry?. I mean, if you want to change the
interface again so that Create is synchronous then that's fine, and we can end
up with two methods if we don't want to cleanup all callers at the same time,
but it should be part of "the" interface. (Anyway, it is easier to change the
implementation to be sync, without changing the API).

In other words, no change should be needed in this file, at least for now.

[gavinp, 2013-01-24 07:38:10]

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

https://codereview.chromium.org/11787007/diff/16001/net/http/http_cache.cc#ne...
net/http/http_cache.cc:716: int rv =
disk_cache::PendingCreateEntry(disk_cache_.get(), key, &new_entry);
On 2013/01/23 22:04:09, rvargas wrote:
> why is this not calling CreateEntry?. I mean, if you want to change the
> interface again so that Create is synchronous then that's fine, and we can end
> up with two methods if we don't want to cleanup all callers at the same time,
> but it should be part of "the" interface. (Anyway, it is easier to change the
> implementation to be sync, without changing the API).
> 
> In other words, no change should be needed in this file, at least for now.

Good point. And if we want to do A/B tests (and we will), it's best to not
change here, or at least make the change minimal.

Hrm, I put all the code in pending_create_entry.cc/.h because it didn't really
need to take a look at any internals of backend_impl, and because I wanted to
have the code in a separate file since it was possible to do so relatively
easily.

I realise we haven't come to agreement about what operations the pending create
object should support (certainly some writes makes sense...), but other than
that, do you have any concerns about the architecture of
pending_create_entry.cc?

[rvargas (doing something else), 2013-01-24 23:26:17]

On 2013/01/24 07:38:10, gavinp wrote:
> https://codereview.chromium.org/11787007/diff/16001/net/http/http_cache.cc
> File net/http/http_cache.cc (right):
> 
>
https://codereview.chromium.org/11787007/diff/16001/net/http/http_cache.cc#ne...
> net/http/http_cache.cc:716: int rv =
> disk_cache::PendingCreateEntry(disk_cache_.get(), key, &new_entry);
> On 2013/01/23 22:04:09, rvargas wrote:
> > why is this not calling CreateEntry?. I mean, if you want to change the
> > interface again so that Create is synchronous then that's fine, and we can
end
> > up with two methods if we don't want to cleanup all callers at the same
time,
> > but it should be part of "the" interface. (Anyway, it is easier to change
the
> > implementation to be sync, without changing the API).
> > 
> > In other words, no change should be needed in this file, at least for now.
> 
> Good point. And if we want to do A/B tests (and we will), it's best to not
> change here, or at least make the change minimal.
> 
> Hrm, I put all the code in pending_create_entry.cc/.h because it didn't really
> need to take a look at any internals of backend_impl, and because I wanted to
> have the code in a separate file since it was possible to do so relatively
> easily.
> 
> I realise we haven't come to agreement about what operations the pending
create
> object should support (certainly some writes makes sense...), but other than
> that, do you have any concerns about the architecture of
> pending_create_entry.cc?

This is not a simple question.

An entry created on the IO thread to avoid thread switching/IO should support
all the basic operations of an entry for some time. By that I mean everything
except sparse support.

I would go as far as to say that the entry should not be created on disk until
the user closes it (or attempts to use sparse methods, or has written beyond a
threshold... 100k? 1M?). Doom/Read/GetTime should be supported directly.

This implies that there should be some tracking of fast(/pending/memory) entries
so that it is possible to open an entry that has not been saved yet.

On the other hand, this means that the code is not so simple anymore... it
requires more control to make sure that we don't end up with a fast and a slow
entry (same key living in both places). But being an optimization, we can simply
decide when to go with a fast entry and when to go with a full entry.