1378714 - RCWN: network wins even if we have cache entry sooner

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[Michal Novotny [:michal]]

If OnCacheEntryAvailable is called before AsyncOpenURI finishes, mCacheEntriesToWaitFor isn't 0 and we don't read the data from the entry because we return early at http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/netwerk/protocol/http/nsHttpChannel.cpp#4471. In this case we simply wait for the data from the network.

I did a quick test in gdb to find out how often this happens: 513 times we didn't read from the entry and 165 times we did. This is a major blocker of RCWN and it explains why about:networking shows that network "wins" more often than cache. This also makes data gathered by pref study more or less invalid.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: [RCWN] ReadFromCache immediately when a cache entry is available

This should fix the problem you point out. However, I still see the network winning more often than not, so there may be another issue here. How did you test this?

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8884833 [details] [diff] [review]
[RCWN] ReadFromCache immediately when a cache entry is available

Review of attachment 8884833 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4467,5 @@
>          return rv;
>      }
>  
> +    if (mCachedContentIsValid && mNetworkTriggered) {
> +        Unused << ReadFromCache(true);

It doesn't seem correct to ignore AwaitingCacheCallbacks here. Instead, we should probably add similar code somewhere around http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/netwerk/protocol/http/nsHttpChannel.cpp#472

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: [RCWN] Make sure we ReadFromCache even when OnCacheEntryAvailable is called before AsyncOpenURI is called

MozReview-Commit-ID: KHUmZp8RuT8

Comment 4

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #1)
> This should fix the problem you point out. However, I still see the network
> winning more often than not, so there may be another issue here. How did you
> test this?

I didn't test it thoroughly. I just found it while working on bug 1378308. I think that whenever network wins and we have an entry with valid content, there is some problem (which was this case). Maybe we should add an assertion to test this?

Comment 5

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #1)
> This should fix the problem you point out. However, I still see the network
> winning more often than not, so there may be another issue here. How did you
> test this?

I get much better results with this patch, but I think the cache is still handicapped by bug 1377223. Landing 1379126 will help a bit because some of the conditional requests will have a chance to be sent.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eacdc01088aead88312c346fc4057c57a0f86f6
Bug 1378714 - [RCWN] Make sure we ReadFromCache even when OnCacheEntryAvailable is called before AsyncOpenURI is called. r=michal

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eacdc01088a
[RCWN] Make sure we ReadFromCache even when OnCacheEntryAvailable is called before AsyncOpenURI is called. r=michal

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eacdc01088a