Closed Bug 1378714 Opened 3 years ago Closed 3 years ago

RCWN: network wins even if we have cache entry sooner

Categories

(Core :: Networking: Cache, enhancement, major)

enhancement
Not set
major

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: michal, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → valentin.gosu
Whiteboard: [necko-active]
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?
Attachment #8884833 - Flags: review?(michal.novotny)
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
Attachment #8884833 - Flags: review?(michal.novotny) → review-
Attachment #8884833 - Attachment is obsolete: true
Attachment #8885015 - Flags: review?(michal.novotny) → review+
(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?
(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.
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
Pushed by valentin.gosu@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/1eacdc01088a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.