Closed Bug 1367551 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: vladan, Assigned: u408661)

References

Details

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

Attachments

(1 file)

<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
Whiteboard: [platform-rel-Facebook], parity-chrome
Whiteboard: [platform-rel-Facebook], parity-chrome → [qf][platform-rel-Facebook], parity-chrome
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.
Assignee: nobody → hurley
Whiteboard: [qf][platform-rel-Facebook], parity-chrome → [necko-active][qf][platform-rel-Facebook], parity-chrome
Whiteboard: [necko-active][qf][platform-rel-Facebook], parity-chrome → [necko-active][qf:p1][platform-rel-Facebook], parity-chrome
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 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)
Attachment #8884056 - Flags: review?(mcmanus) → review?(honzab.moz)
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155004/#review167468

this was a fair bit of work - but I think worth it. Thank you. I'll r+ it for h2 specifically and as a generally good arrangement.

I'd like honza to also review the rearrangement of the cache validators just from a regression pov.
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155004/#review167470
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155004/#review167472
Attachment #8884056 - Flags: review?(mcmanus)
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155006/#review167474
Attachment #8884056 - Flags: review?(mcmanus) → review+
Comment on attachment 8884056 [details]
Bug 1367551 - Cancel pushes when we already have the item in cache.

https://reviewboard.mozilla.org/r/155006/#review167752

::: netwerk/protocol/http/Http2Session.cpp:1855
(Diff revision 2)
> +    // unneeded (we already have it in our local regular cache). See bug 1367551.
> +    nsCOMPtr<nsICacheStorageService> css =
> +      do_GetService("@mozilla.org/netwerk/cache-storage-service;1");
> +    mozilla::OriginAttributes oa;
> +    pushedStream->GetOriginAttributes(&oa);
> +    RefPtr<LoadContextInfo> lci = GetLoadContextInfo(false, oa);

can a push ever be anonymous?

::: netwerk/protocol/http/Http2Session.cpp:1873
(Diff revision 2)
> +    // handler to create any URIs, this will work just fine here. Don't try this
> +    // at home, though, kids. I'm a trained professional.
> +    if (NS_SUCCEEDED(Http2Stream::MakeOriginURL(spec, pushedURL))) {
> +      LOG3(("Http2Session::RecvPushPromise %p check disk cache for entry", self));
> +      RefPtr<CachePushCheckCallback> cpcc = new CachePushCheckCallback(self, promisedID, pushedStream->GetRequestString());
> +      if (NS_FAILED(ds->AsyncOpenURI(pushedURL, EmptyCString(), nsICacheStorage::OPEN_READONLY|nsICacheStorage::OPEN_SECRETLY, cpcc))) {

this is not a requirement but why exactly can't the check be processed on the cache io thread?  (i.e. add the OPEN_MULTITHREADED - or what is the name exactly -  flag).  note that then the OnCacheEntryCheck callback may be called on either the cache io thread or the thread that called asyncOpenURI on the storage.

we are trying hard to avoid main thread where ever possible.
Attachment #8884056 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 8884056 [details]
> Bug 1367551 - Cancel pushes when we already have the item in cache.
> 
> https://reviewboard.mozilla.org/r/155006/#review167752
> 
> ::: netwerk/protocol/http/Http2Session.cpp:1855
> (Diff revision 2)
> > +    // unneeded (we already have it in our local regular cache). See bug 1367551.
> > +    nsCOMPtr<nsICacheStorageService> css =
> > +      do_GetService("@mozilla.org/netwerk/cache-storage-service;1");
> > +    mozilla::OriginAttributes oa;
> > +    pushedStream->GetOriginAttributes(&oa);
> > +    RefPtr<LoadContextInfo> lci = GetLoadContextInfo(false, oa);
> 
> can a push ever be anonymous?

I mean, I suppose it's possible that we would be pushed a resource that we would request with LOAD_ANONYMOUS, but I don't think there's any way to know at this point whether or not that would happen.

> ::: netwerk/protocol/http/Http2Session.cpp:1873
> (Diff revision 2)
> > +    // handler to create any URIs, this will work just fine here. Don't try this
> > +    // at home, though, kids. I'm a trained professional.
> > +    if (NS_SUCCEEDED(Http2Stream::MakeOriginURL(spec, pushedURL))) {
> > +      LOG3(("Http2Session::RecvPushPromise %p check disk cache for entry", self));
> > +      RefPtr<CachePushCheckCallback> cpcc = new CachePushCheckCallback(self, promisedID, pushedStream->GetRequestString());
> > +      if (NS_FAILED(ds->AsyncOpenURI(pushedURL, EmptyCString(), nsICacheStorage::OPEN_READONLY|nsICacheStorage::OPEN_SECRETLY, cpcc))) {
> 
> this is not a requirement but why exactly can't the check be processed on
> the cache io thread?  (i.e. add the OPEN_MULTITHREADED - or what is the name
> exactly -  flag).  note that then the OnCacheEntryCheck callback may be
> called on either the cache io thread or the thread that called asyncOpenURI
> on the storage.
> 
> we are trying hard to avoid main thread where ever possible.

The way I understood opening items without OPEN_MULTITHREADED is that the check would happen on the thread that called AsyncOpenURI (the socket thread, in this case). Which is exactly what we want - I potentially need to access the h2 session to cancel the push, and that needs to happen on the socket thread. If I'm wrong, and the check always happens on the main thread without OPEN_MULTITHREADED, regardless of what thread called AsyncOpenURI, then there needs to be a little rearranging that happens to avoid the main thread (I think it's just a single dispatch to the socket thread at the end of the check, when I cancel the push, but there may be a couple more things I need to rearrange as well).
Flags: needinfo?(honzab.moz)
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #20)
> (In reply to Honza Bambas (:mayhemer) from comment #19)
> > Comment on attachment 8884056 [details]
> > Bug 1367551 - Cancel pushes when we already have the item in cache.
> > 
> > https://reviewboard.mozilla.org/r/155006/#review167752
> > 
> > ::: netwerk/protocol/http/Http2Session.cpp:1855
> > (Diff revision 2)
> > > +    // unneeded (we already have it in our local regular cache). See bug 1367551.
> > > +    nsCOMPtr<nsICacheStorageService> css =
> > > +      do_GetService("@mozilla.org/netwerk/cache-storage-service;1");
> > > +    mozilla::OriginAttributes oa;
> > > +    pushedStream->GetOriginAttributes(&oa);
> > > +    RefPtr<LoadContextInfo> lci = GetLoadContextInfo(false, oa);
> > 
> > can a push ever be anonymous?
> 
> I mean, I suppose it's possible that we would be pushed a resource that we
> would request with LOAD_ANONYMOUS, but I don't think there's any way to know
> at this point whether or not that would happen.
> 
> > ::: netwerk/protocol/http/Http2Session.cpp:1873
> > (Diff revision 2)
> > > +    // handler to create any URIs, this will work just fine here. Don't try this
> > > +    // at home, though, kids. I'm a trained professional.
> > > +    if (NS_SUCCEEDED(Http2Stream::MakeOriginURL(spec, pushedURL))) {
> > > +      LOG3(("Http2Session::RecvPushPromise %p check disk cache for entry", self));
> > > +      RefPtr<CachePushCheckCallback> cpcc = new CachePushCheckCallback(self, promisedID, pushedStream->GetRequestString());
> > > +      if (NS_FAILED(ds->AsyncOpenURI(pushedURL, EmptyCString(), nsICacheStorage::OPEN_READONLY|nsICacheStorage::OPEN_SECRETLY, cpcc))) {
> > 
> > this is not a requirement but why exactly can't the check be processed on
> > the cache io thread?  (i.e. add the OPEN_MULTITHREADED - or what is the name
> > exactly -  flag).  note that then the OnCacheEntryCheck callback may be
> > called on either the cache io thread or the thread that called asyncOpenURI
> > on the storage.
> > 
> > we are trying hard to avoid main thread where ever possible.
> 
> The way I understood opening items without OPEN_MULTITHREADED is that the
> check would happen on the thread that called AsyncOpenURI (the socket
> thread, in this case). Which is exactly what we want - I potentially need to
> access the h2 session to cancel the push, and that needs to happen on the
> socket thread. If I'm wrong, and the check always happens on the main thread
> without OPEN_MULTITHREADED, regardless of what thread called AsyncOpenURI,
> then there needs to be a little rearranging that happens to avoid the main
> thread (I think it's just a single dispatch to the socket thread at the end
> of the check, when I cancel the push, but there may be a couple more things
> I need to rearrange as well).

W/o the flag oncheck is called on the thread that called asyncOpenURI.  so you are safe.  but add an assertion to the method, maybe?
Flags: needinfo?(honzab.moz)
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/324cdf85739c
Cancel pushes when we already have the item in cache. r=mayhemer,mcmanus
https://hg.mozilla.org/mozilla-central/rev/324cdf85739c
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
No longer depends on: 1390824
Performance Impact: --- → P1
Whiteboard: [necko-active][qf:p1][platform-rel-Facebook], parity-chrome → [necko-active][platform-rel-Facebook], parity-chrome
You need to log in before you can comment on or make changes to this bug.