Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Cancel HTTP2 push when the resource is already in the disk cache

NEW
Assigned to

Status

()

Core
Networking: HTTP
2 months ago
9 days ago

People

(Reporter: vladan, Assigned: nwgh)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-active][qf:p1][platform-rel-Facebook], parity-chrome)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
<nwgh>	we don't inspect the disk cache to determine if we should cancel a push or not
<nwgh>	so if you push anything, as long as it's not in our push cache, we'll accept it (based on origin checks, etc)

Pushing resources the client already has in their disk cache is a waste of bandwidth and causes page loading time regressions.

Oddly enough, Chrome's HTTP2 push implementation had the same bug. They fixed it in a recent release though https://bugs.chromium.org/p/chromium/issues/detail?id=232040#c43
(Reporter)

Updated

2 months ago
Whiteboard: [platform-rel-Facebook], parity-chrome
Whiteboard: [platform-rel-Facebook], parity-chrome → [qf][platform-rel-Facebook], parity-chrome
(Reporter)

Comment 1

2 months ago
This bug matters because it prevents Facebook from rolling out HTTP2 Push (a performance win) to its Firefox users.

We did an A/B experiment where we Pushed the set of static resources needed to display the initial view of the FB newsfeed, and we discovered that HTTP2 Push was actually a loading time regression for Firefox users because we wasted time transmitting resources that were already in the user's regular disk cache.
(Reporter)

Updated

2 months ago
Blocks: 1288602
Assignee: nobody → hurley
Whiteboard: [qf][platform-rel-Facebook], parity-chrome → [necko-active][qf][platform-rel-Facebook], parity-chrome
Duplicate of this bug: 1368661

Updated

2 months ago
Whiteboard: [necko-active][qf][platform-rel-Facebook], parity-chrome → [necko-active][qf:p1][platform-rel-Facebook], parity-chrome
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4596d1af3a30aa4fb503d2d8eae21bf17791e16
a few questions

 - are we confident we can access the cache service in a blocking fashion like that on the socket thread? Seems like a blocker..

 - is existence the right check? what if it is expired on disk?

-  or simply that the pushed one is newer? (or even uncachable?)

my bz folder is a mess.. please ni me :)
Flags: needinfo?(hurley)
(In reply to Patrick McManus [:mcmanus] from comment #5)
> a few questions
> 
>  - are we confident we can access the cache service in a blocking fashion
> like that on the socket thread? Seems like a blocker..

Yep, the "Exists" is simply an in-memory lookup. I discussed with Michal in SFO, and he was confident it was ok.

>  - is existence the right check? what if it is expired on disk?
> 
> -  or simply that the pushed one is newer? (or even uncachable?)

These did pop into my mind, but given the reported use case (FB doesn't want to waste bits pushing stuff we already have cached, and their "if it's cached, it's valid" story seems pretty solid to me based on my knowledge of things), I figured this was an ok tradeoff. I'm willing to entertain the possibility my calculus was wrong, there :)

Looking at chrome's code, it looks like they might be doing something similar. There's certainly no logic in their "cache transaction finished" callback that does validity checks. Perhaps their cache transaction does all the validity logic internally, and so I'm missing that knowledge.
Flags: needinfo?(hurley) → needinfo?(mcmanus)
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #6)
> (In reply to Patrick McManus [:mcmanus] from comment #5)
> Looking at chrome's code, it looks like they might be doing something
> similar. There's certainly no logic in their "cache transaction finished"
> callback that does validity checks. Perhaps their cache transaction does all
> the validity logic internally, and so I'm missing that knowledge.

Oh, heh, I just looked at their code. They have LOAD_SKIP_CACHE_VALIDATION in their flags when they go to hit the cache (as well as LOAD_ONLY_FROM_CACHE). Whether those together equate to "don't hit the network to validate" or "don't do *any* validation" is still unknown to me, but that sounds pretty similar to what I've done, on the face of it.
well FB is all about immutable.. so for them exists == fresh.. but I'm not sure that's something we should codify.

is it super hard to make a round trip and let the cache service figure out validity asynchronously?
Flags: needinfo?(mcmanus)
Making the round trip for a full cache open wouldn't be horrible in terms of code. What's One More Callback? :D

As for figuring out validity - does our cache service actually do that at all right now? I was under the impression that all that logic existed in the channel. (That was another reason I wanted to avoid the full open if possible -- duplicating this logic and/or factoring it out of the channel are both rather fraught options.)
Flags: needinfo?(mcmanus)
well, cache correctness is based on a combination of the request (aka push promise) and the cached response headers.. vary and etc.. that is all currently done in the channel but I would think it would be relatively easy to extract it.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #10)
> well, cache correctness is based on a combination of the request (aka push
> promise) and the cached response headers.. vary and etc.. that is all
> currently done in the channel but I would think it would be relatively easy
> to extract it.

Yeah, it's not exactly "easy" to extract. I'm working my way through it, and it will probably be doable, but it's not straightforward by any stretch of the imagination :)

Comment 12

9 days ago
mozreview-review
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155006/#review161630
Attachment #8884056 - Flags: review?(mcmanus)
You need to log in before you can comment on or make changes to this bug.