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)
Core
Networking: HTTP
Tracking
()
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
Reporter | ||
Updated•7 years ago
|
Whiteboard: [platform-rel-Facebook], parity-chrome
Updated•7 years ago
|
Whiteboard: [platform-rel-Facebook], parity-chrome → [qf][platform-rel-Facebook], parity-chrome
Reporter | ||
Comment 1•7 years 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•7 years ago
|
Blocks: Meta-Hasal-Facebook
Updated•7 years ago
|
Assignee: nobody → hurley
Whiteboard: [qf][platform-rel-Facebook], parity-chrome → [necko-active][qf][platform-rel-Facebook], parity-chrome
Updated•7 years ago
|
Whiteboard: [necko-active][qf][platform-rel-Facebook], parity-chrome → [necko-active][qf:p1][platform-rel-Facebook], parity-chrome
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
(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•7 years 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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58272790dc45ce5d9447e9f61eb11b6203a9730a
Updated•7 years ago
|
Attachment #8884056 -
Flags: review?(mcmanus) → review?(honzab.moz)
Comment 15•7 years 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/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 16•7 years 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/155004/#review167470
Comment 17•7 years 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/155004/#review167472
Updated•7 years ago
|
Attachment #8884056 -
Flags: review?(mcmanus)
Comment 18•7 years 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/#review167474
Attachment #8884056 -
Flags: review?(mcmanus) → review+
Comment 19•7 years 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/#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+
Assignee | ||
Comment 20•7 years ago
|
||
(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)
Comment 21•7 years ago
|
||
(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)
Assignee | ||
Comment 22•7 years ago
|
||
Try run with thread assert added: https://treeherder.mozilla.org/#/jobs?repo=try&revision=418860bea2656d5e8725673b3adb70982ac82139
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/324cdf85739c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•2 years ago
|
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.
Description
•