Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(203)

Issue 11787007: Race Create vs SendRequest on cache misses. (Closed)

Created:
7 years, 11 months ago by gavinp
Modified:
7 years, 10 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, gavinp+disk_chromium.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : cleaner #

Patch Set 3 : move it into the disk cache #

Total comments: 4

Patch Set 4 : retain http_cache, fix lifetime #

Patch Set 5 : lose a race #

Unified diffs Side-by-side diffs Delta from patch set Stats (+316 lines, -3 lines) Patch
M net/base/net_error_list.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A net/disk_cache/pending_create_entry.h View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
A net/disk_cache/pending_create_entry.cc View 1 2 3 1 chunk +278 lines, -0 lines 0 comments Download
M net/http/http_cache.cc View 1 2 3 4 3 chunks +6 lines, -3 lines 0 comments Download
M net/net.gyp View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
gavinp
Ricardo, I know this isn't ready for landing, but I'd appreciate you taking a peek ...
7 years, 11 months ago (2013-01-08 21:16:21 UTC) #1
rvargas (doing something else)
I don't really think that testing is the more concerning part here. This CL logically ...
7 years, 11 months ago (2013-01-09 20:01:51 UTC) #2
pasko-google - do not use
On 2013/01/09 20:01:51, rvargas wrote: > I still think that this is the wrong place ...
7 years, 11 months ago (2013-01-09 22:46:14 UTC) #3
rvargas (doing something else)
On 2013/01/09 22:46:14, pasko wrote: > On 2013/01/09 20:01:51, rvargas wrote: > > I still ...
7 years, 11 months ago (2013-01-10 00:21:02 UTC) #4
pasko-google - do not use
On 2013/01/10 00:21:02, rvargas wrote: > On 2013/01/09 22:46:14, pasko wrote: > > On 2013/01/09 ...
7 years, 11 months ago (2013-01-10 14:05:44 UTC) #5
pasko-google - do not use
On 2013/01/10 14:05:44, pasko wrote: > current UMA stats on desktop suggest that about 0.04% ...
7 years, 11 months ago (2013-01-10 16:14:27 UTC) #6
rvargas (doing something else)
To close the loop, I'm not saying we should not care about this issue, I'm ...
7 years, 11 months ago (2013-01-10 19:09:12 UTC) #7
gavinp
Ricardo, Can you take a brief look at this architecturally? - Gavin
7 years, 11 months ago (2013-01-23 15:08:30 UTC) #8
rvargas (doing something else)
https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_create_entry.cc File net/disk_cache/pending_create_entry.cc (right): https://codereview.chromium.org/11787007/diff/16001/net/disk_cache/pending_create_entry.cc#newcode18 net/disk_cache/pending_create_entry.cc:18: // to true if it receives any operation we ...
7 years, 11 months ago (2013-01-23 22:04:09 UTC) #9
gavinp
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#newcode716 net/http/http_cache.cc:716: int rv = disk_cache::PendingCreateEntry(disk_cache_.get(), key, &new_entry); On 2013/01/23 22:04:09, ...
7 years, 11 months ago (2013-01-24 07:38:10 UTC) #10
rvargas (doing something else)
7 years, 11 months ago (2013-01-24 23:26:17 UTC) #11
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.

Powered by Google App Engine
This is Rietveld 408576698