Impl: - request context (load group) blocks and queues Tail requests - channels call on its request context to check if they (the channels) must wait for unblock either - from AsyncOpen when the Tail class is set - from result of TP decision (classification) and found tracking listed - when unblocked, we call 'Continue(nsresult result)' on each channel that should finish opening the channel on result=OK or cancel it on result!=OK

(doesn't throttle/tail leaders/unblocked/urgent)

- adds nsIClassOfService::Tail flag - when set on a channel, it's not opened until all request in the same request context NOT having the Tail flag delivered OnStopRequest - tailing is not performed on channel with any of Leader/Unblocked/UrgentStart class flags set to not block onload - there are following cut points, each of them may cause the channel opening process to be suspended until the untail callback on the channel is triggered: child asyncOpen parent asyncOpen parent after result of TP annotation - "untailing" is performed the following way: - "adding" or "removing" a non-tailed requests (a simple counter, similar to how we count blocking transactions, but here for a channel between asyncOpen and OnStopRequest delivery) re-schedules a timer that triggers the tail unblock callback on all channels - the delay is calculated as max((untailed request count + 1) * 600ms, 10s) - the timer has been added for two reasons: - similarly to how we are handling throttling, there can be gaps during the page load processing when there are simply no (non-tailed) requests scheduled. these gaps would unblock the tail too soon - if there is a video or audio being played (a single resource constantly loading) we don't want to block the tail on it ; on the child process media requests are (after opening!) marked as LOAD_BACKGROUND, unfortunately this load flags change is not propagated to the parent process, otherwise I would use that to filter those out as "non-tailed" - one a bit out-of-context change is that we don't lower priority of TP resources that are Leader/Unblocked/UrgentStart for the same reason as not tailing them (maybe move that to bug 1383234?) You may ask why I set the Throttleable and Tail flags even when there is any of Leader/Unblocked/UrgentStart flags set. Setting the Tail/Throttle flag won't have any effect then, but I think to have the information there is a good thing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb5de6ed8a12d7afa29874d7f6ccefce71fb6f72

better try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=485f16c0b154e9f95841e455b54efe4109a6aacf

even better try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5d76f16f9ce6860d454bdbd634a1bd32c517865

Comment on attachment 8889532 [details] [diff] [review] v1 Review of attachment 8889532 [details] [diff] [review]: ----------------------------------------------------------------- comment 6, c3 seems to show a permafail caused by this

(In reply to Patrick McManus [:mcmanus] from comment #7) > Comment on attachment 8889532 [details] [diff] [review] > v1 > > Review of attachment 8889532 [details] [diff] [review]: > ----------------------------------------------------------------- > > comment 6, c3 seems to show a permafail caused by this Yes, it's likely the test needs an update. Can you provide feedback on the patch, please, so we can save some rtts? Thanks.

- fixes few printf formatting mistakes - the failing test was modified: the test (between else) checks we lower the priority of an xhr's channel that is made to a tracking domain, but this patch changes the prioritization condition: we don't do that for Unblocked resources, which xhrs definitely are, all of them ; hence, the xhr test is removed OTOH, thinking of it as I write this comment, maybe unblocked resources should not fall into the "not eligible for tail/low-prio" treating? Not sure. https://treeherder.mozilla.org/#/jobs?repo=try&revision=29f166bca150cd992d0cc14b9167ec19da2b27ea

(In reply to Honza Bambas (:mayhemer) from comment #10) > OTOH, thinking of it as I write this comment, maybe unblocked resources > should not fall into the "not eligible for tail/low-prio" treating? Not > sure. I think we should not tail them but lower the priority. I'll update the patch.

I'll ask for r after the merge, everyone is too busy and this is too large to r quickly and probably also a bit risky to land this late just before beta. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7b5236ad12939171367d0559c3cada538ab504

(merged) for details see comment 4.

Comment on attachment 8895407 [details] [diff] [review] v1.2 Forwarding to Dragana, please see comment 4 for details or just ping me on IRC. Thanks!

Comment on attachment 8895407 [details] [diff] [review] v1.2 Review of attachment 8895407 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/RequestContextService.cpp @@ +164,5 @@ > +RequestContext::ScheduleUnblock() > +{ > + if (mUntailTimer) { > + mUntailTimer->Cancel(); > + } do we need to restart timer for each single resource being added/removed? Maybe set it once and on the timer notification reschedule if mNonTailRequests>0? just a suggestion... ::: netwerk/base/nsChannelClassifier.cpp @@ -1018,5 @@ > SetIsTrackingResourceHelper(channel); > if (CachedPrefs::GetInstance()->IsLowerNetworkPriority()) { > LowerPriorityHelper(channel); > } > - SetThrottleableHelper(channel); SetThrottleableHelper is not used any more ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +3190,5 @@ > } > > // We have to make sure to drop the references to listeners and callbacks > // no longer needed > + RemoveAsNonTailRequest(); move it above the comment, since the comment is about the line below. ::: netwerk/protocol/http/HttpBaseChannel.h @@ +427,5 @@ > // Callback on main thread when NS_AsyncCopy() is finished populating > // the new mUploadStream. > void EnsureUploadStreamIsCloneableComplete(nsresult aStatus); > > + // Called exclusively only from AsyncOpen. also after nsURIClassifier callbacks? ::: netwerk/protocol/http/nsHttpChannel.h @@ +673,5 @@ > > + // A function we trigger when untail callback is trigger by our request > + // context in case this channel was tail-blocked. > + nsresult (nsHttpChannel::*mOnTailUnblock)(); > + // Called on untail when tailed during AsyncOpen execution. also after nsURIClassifier callbacks?

one more thing, I think that 600ms and 20s is too high, maybe it would be good to have some user testing to see how that looks for them. Tracking resources are going to show up after 20s maybe that is going to look strange, as if page took 20s and more to load and the first part is done in 3s.

(In reply to Dragana Damjanovic [:dragana] from comment #15) > do we need to restart timer for each single resource being added/removed? > > Maybe set it once and on the timer notification reschedule if > mNonTailRequests>0? I want to shorten the time whenever we remove a resource. But rescheduling when a resource is added could be optimzied, yes. But I'd rather do this in a followup bug regarding the time window for 57 freeze. I don't think the patch adds a major noticeable perf regression with this. Thanks for r!

(In reply to Dragana Damjanovic [:dragana] from comment #16) > one more thing, I think that 600ms and 20s is too high, maybe it would be > good to have some user testing to see how that looks for them. Tracking > resources are going to show up after 20s maybe that is going to look > strange, as if page took 20s and more to load and the first part is done in > 3s. We reach the 20s limit only when there is 20s / 0.6s = 34 resources still in progress of loading. Note we never tail resources referenced from <head> that may block DOMContentLoaded or first paint. I'm thinking myself of shorting these times down to 300ms and 10s (a half), but it's not based on any analytical data, just my feeling.

Comment on attachment 8895407 [details] [diff] [review] v1.2 Not ready yet. - tailing of local-blocklisted resources must add Tail cos - OTOH, it's done too soon, we should allow full classification that can cancel the channel - tailing wait has to be canceled when the channel is cancled - and at the moment I'm not sure we correctly recognize what to tail and what not, cos flags and classification may not be all we need

not asking r until I'm sure I deal with the trackers correctly.

This is a way better working version! I found out (bless you logan!) that there are still 69 blockers on my favorite testing page [1] that are finishing loading after DOMContentLoaded _and_ *are* fully recognized as trackers before we create the transaction. But those 69 trackers are added dynamically and are marked Unblocked, for which I forbid tailing. Hence, the next move here is to find out if the Unblocked flag is needed for all of them or if we under certain conditions may allow tailing for such resources. I think it should be a followup to this bug. I'll clean the patch up and ask for another r since some details has changed. Regardless the Unblocked limitation, I can see few tracker images being correctly blocked before my chosen hero element! And it seems it has some influence, but that is of course hard w/o WPT heavy testing. [1] http://edition.cnn.com/2017/07/20/fashion/2018-pirelli-calendar-all-black-tim-walker-alice/index.html

Comment on attachment 8899820 [details] [diff] [review] v2 Found few small issue with this patch that need some care.

(This an updated comment 23) What _has NOT_ changed since the last patch you reviewed: - the overall tail callback referencing and non-tail requests counting - tailing is disabled on a channel when any of Unblocked, Leader or UrgentStart are set What has changed: - each request context (1-1 relation with each individual iframe, but recycled for top level documents) is notified about a load start and DOMContentLoaded event - the delay quantum has two values: * between document load begin and DOMContentLoaded it's left at 600ms, which is IMO reasonable * after DOMContentLoaded it's lowered down to only 20ms to still keep the tailed requests a bit lowered on scheduling priority but also let any tracker scripts be loaded now so that any dynamic content delivery is no longer blocked - probably the biggest change: we now mark js loads as Unblocked (which would disable tailing otherwise) for _other than_ async scripts *) ; before this patch we were marking other than defer scripts as Unblocked, but I'm not sure why (no comment explanation..) and it didn't seem reasonable to me - XHR from a tracker is dropped the Unblocked flag to allow tailing - rescheduling of the untail timer was a bit optimized (had you something like this in mind?) And v2->v3: - supported only on the parent (we don't have an easy access to preferences on the child) ; this involves moving some stuff from base channel to nshttp. - thread assertions added - slight improvement of the "is a request blocked" condition and handling - a tail-blocked requests (queued for untail) is removed from the tail queue when canceled - calling RemoveNonTailRequest on the request context from nsHttpChannel's dtor on the main thread - some comments cleanup *) async scripts are either those having the 'async' attribute set or any being dynamically added from any other inline, sync or deferred script. async scripts don't block DOMContentLoaded, hence it's OK to allow delaying them (when later found to be trackers) before DOMContentLoaded has fired With this patch I can speed up cnn hero images (analytically confirmed!) and somewhat bbc home page (this only subjectively) w/o a noticeable regression in the overall loading time.

(In reply to Honza Bambas (:mayhemer) from comment #26) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=94b2dad196323f5cf4e24d15a516a148fede4215 This is v3 green try!

Comment on attachment 8900351 [details] [diff] [review] v3 Review of attachment 8900351 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/RequestContextService.cpp @@ +69,5 @@ > + TimeStamp mUntailAt; > + > + // false when the document associated with this rc started to load > + // true after DOMContentLoaded event has been fired for the document > + // Note that each (i)frame has its own request context I am not sure I understand this comment. I figured what it does but comment can be fixed. The value of mAfterDOMContentLoaded is false before BeginLoad as well. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +3056,5 @@ > + LOG(("HttpBaseChannel::AddAsNonTailRequest this=%p, rc=%p, already added=%d", > + this, mRequestContext.get(), (bool)mAddedAsNonTailRequest)); > + > + if (!mAddedAsNonTailRequest) { > + mRequestContext->AddNonTailRequest(); this function will be called on the main thread, right? can you add assertion.

Fixed wrong assertion: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6444bdf29f8aa36d9178f554de7965fbdc84c50

Can't block async scripts, or we regress bug 1014058. Must block defer scripts, or we regress bug 1053321. Hence, and also as I don't have any heuristic in pocket, I'll revert the condition as it was before this patch and resubmit once more.

Comment on attachment 8901432 [details] [diff] [review] v3 -> v4.1 Still bad. Breaks bug 503481.

try in maintenance mode...

This should be the final one, pgo regressions on talos are not considered showstoppers. For talos run see comment 40, which is functionally identical to the patch pushed there. Compare: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=84ee8d46436c&newProject=try&newRevision=c7bcbe1b0443&framework=1&showOnlyConfident=1 - there are now 3 tailing related class of service flags: - Tail: when none of Leader, Unblocked, UrgentStart, TailForbidden is also set, tailing is performed on the channel - TailAllowed: when the load is found to be a tracker during nsHttpChannel classification, Unblocked is unset and Tail is set on the channel to perform tailing - TailForbidden: when set, tailing is explicitly disabled, no other condition or flag combination may allow tailing This way we have a very fine control of when tailing is allowed or enforced while we don't have to mess with the Leader and Unblocked prioritization flags. I moved to this solution because only Leader and Unblocked flags were not enough to carry the do-tail/maybe-tail/never-tail preconditions well. how is this used? - <head><script> is only set Leader, it will never tail - <head><script defer> and <body><script defer> are only set TailForbidden since we want them to start loading AFTER leaders while also not to block anything else (bug 1053321) but finish them ASAP as they block DOMContentLoaded - <head><script async>, <body><script async> and <body><script> are set Unblocked (bug 1014058) - <head><script async> and <body><script async> are _added_ TailAllowed, since when those are later found to be 3rd-party trackers, we must drop Unblocked and set Tail to perform tailing; that is done inside nsHttpChannel - this makes <body><script> to never tail but be loaded unblocked, parallel with leaders - general requests (including images) w/o any class-of-service flags that are found to be tracking are set Tail inside nsHttpChannel what performs tailing on them - XHR from a tracker and fetch() from a tracker are set Tail (and unset Unblocked) during the underlying channel setup generally speaking, Tail is set only by a code that _knows_ the load is a tracker or tailing is explicitly desired and is not explicitly or implicitly disallowed. (note that <script async defer> == <script async>) - small change: after DOMContentLoaded the untail delay is 0ms when there are no non-tailed requests active ; I realized that we could block a lonely tracking request for 100+ms (one or two checks for tail blocking), note that the delay is reset when any new request is created, even a potential tracker - I changed the after-DCL delay quantum to 100ms, since I can see a benefit during display content load happening after DCL (more trackers can be delay with it); note that I was testing with the quantum and max being all 20s and no long delays were encountered but if you oppose I can go back to 20ms or something similar - some ScriptLoader LOGs added. I will also submit a patch of pieces that only has majorly changed since the last time, so that it will be easier to review. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed6d58859b6de9b13ba99d3ce9f0101e1c67ed79

Comment on attachment 8901871 [details] [diff] [review] v6.0.1 Ha! One more small detail found when I was walking the changes for the reduced patch I promised. Will update shortly!

Comment 41 applies EXCEPT: - Unblocked flag is not dropped from TailAllowed trackers - I rather modified nsHttpChannel::EligibleForTailing to allow tailing when both Unblocked and TailAllowed are set This pretty much simplifies code of nsHttpChannel::Connect and also doesn't change the prioritization flags at all. (The previous patch was dropping it even for non-trackers!) https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aca12d35a319dec295530787693e1d9ce26ceef Talos: Baseline vs feature on: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=44707055b9fb&newProject=try&newRevision=6000953c6a08&framework=1&showOnlyImportant=0 Baseline vs feature off: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=44707055b9fb&newProject=try&newRevision=70452ca3266c&framework=1&showOnlyImportant=0

Comment on attachment 8901892 [details] [diff] [review] v6.1 Review of attachment 8901892 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xhr/XMLHttpRequestMainThread.cpp @@ +2625,5 @@ > + if (cos) { > + // To allow tailing, we also need to drop the Unblocked flag. > + cos->ClearClassFlags(nsIClassOfService::Unblocked); > + cos->AddClassFlags(nsIClassOfService::Throttleable | > + nsIClassOfService::Tail); we should probably set TailAllowed here instead of removing the Unblocked flag, but this is really only a detail..

Comment on attachment 8901892 [details] [diff] [review] v6.1 https://treeherder.mozilla.org/#/jobs?repo=try&author=honzab.moz@firemni.cz&fromchange=25d1ddb1cd1f07e9ae5641465219f0a6261c5617&tochange=a9912274b61b8e9c8c0fa97030095cfbb9038565

(In reply to Honza Bambas (:mayhemer) from comment #43) > Created attachment 8901892 [details] [diff] [review] > v6.1 > Talos: > > Baseline vs feature on: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=44707055b9fb&newProject=try&newR > evision=6000953c6a08&framework=1&showOnlyImportant=0 > > Baseline vs feature off: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=44707055b9fb&newProject=try&newR > evision=70452ca3266c&framework=1&showOnlyImportant=0 The "feature on" does not have so many results :) Have you run webpage tests? Maybe talk to Valentin to use presto. It would be good to see if this delays influence the perception when the page is fully loaded.

(In reply to Dragana Damjanovic [:dragana] from comment #47) > > The "feature on" does not have so many results :) Sorry, those were repushed: OFF: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=25d1ddb1cd1f&newProject=try&newRevision=a9912274b61b&framework=1&showOnlyConfident=1 ON: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=25d1ddb1cd1f&newProject=try&newRevision=ad03ea3dbdf3&framework=1&showOnlyConfident=1 > > Have you run webpage tests? Maybe talk to Valentin to use presto. It would > be good to see if this delays influence the perception when the page is > fully loaded. Planed. But I'd like to land this now regardless test results. We can tune up later (up to the beta phase) or turn off completely when found harmful.

After presto tests, maybe writing to dev-platform tell people to pay attention if they see that some parts of a page are rendered late. I am afraid of regressing perception of page load and that nobody will notice it. Maybe ask the test team to test alexa 100 manually... we should talk on the next necko meeting.

(In reply to Dragana Damjanovic [:dragana] from comment #49) > After presto tests, maybe writing to dev-platform tell people to pay > attention if they see that some parts of a page are rendered late. I would like to let this feature unannounced for a short time. I don't want to bias people's perception. I will be offline next week and week after, so I won't be able to do any announcements. I can find someone (maybe Jason when he's back?) to do it, tho. > I am > afraid of regressing perception of page load and that nobody will notice it. > > Maybe ask the test team to test alexa 100 manually... > > we should talk on the next necko meeting. I told Gary to run WPT tests, he promised last week to run it on his private instance.

Comment on attachment 8901892 [details] [diff] [review] v6.1 Review of attachment 8901892 [details] [diff] [review]: ----------------------------------------------------------------- Please open a bug to track testing. and maybe necessary pref updates. ::: dom/xhr/XMLHttpRequestMainThread.cpp @@ +2625,5 @@ > + if (cos) { > + // To allow tailing, we also need to drop the Unblocked flag. > + cos->ClearClassFlags(nsIClassOfService::Unblocked); > + cos->AddClassFlags(nsIClassOfService::Throttleable | > + nsIClassOfService::Tail); yes, please update. If tailing is turned off the behavior should be the same as before. ::: modules/libpref/init/all.js @@ +2252,5 @@ > +pref("network.http.tailing.delay-quantum", 600); > +// The same as above, but applied after the document load reached DOMContentLoaded event. > +pref("network.http.tailing.delay-quantum-after-domcontentloaded", 100); > +// Upper limit for the calculated delay, prevents long standing and comet-like requests > +// tail forever. please add that this is in milliseconds. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +6798,5 @@ > } > + if (EligibleForTailing()) { > + RemoveAsNonTailRequest(); > + } else { > + AddAsNonTailRequest(); shouldn't you check if tailing is enabled? Maybe add the check inside AddAsNonTailRequest and other...

(In reply to Dragana Damjanovic [:dragana] from comment #51) > Comment on attachment 8901892 [details] [diff] [review] > v6.1 > > Review of attachment 8901892 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please open a bug to track testing. you mean QA testing? > and maybe necessary pref updates. not sure what that means. > yes, please update. If tailing is turned off the behavior should be the same > as before. will do, another try push is not necessary for that since this is more restrictive than when updated. > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +6798,5 @@ > > } > > + if (EligibleForTailing()) { > > + RemoveAsNonTailRequest(); > > + } else { > > + AddAsNonTailRequest(); > > shouldn't you check if tailing is enabled? > > Maybe add the check inside AddAsNonTailRequest and other... there is no harm when this is collected even when off. note that also the timer is scheduled only when there are some tailed requests, and there are none when the pref is off. Thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b37a0bd71bbb Allow postponing of unimportant resources opening during page load, class-of-service Tail flag. r=dragana

(Turned to have a good impact)

This comes late, but I still think it might be included to rel notes for at least 58: Release Note Request (optional, but appreciated) [Why is this notable]: this significantly changes scheduling (prioritization) of regular vs tracking requests during a web page load and could lead to potential breakage of not-well built web pages (see some of the dependencies) [Affects Firefox for Android]: probably yes [Suggested wording]: Loading tracking asynchronous scripts and images only after majority of regular site resources have loaded. [Links (documentation, blog post, etc)]: https://www.janbambas.cz/firefox-57-delays-requests-tracking-domains/

Gerry, want this for a 58 release note?

Or, we could add it to the 57 notes. Ritu, what do you think? It seems more accurate that way.

I think 57 release notes will be old/retire in a week. Adding it to 58 seems more appropriate.

I've added it in 58 gdoc.

(In reply to Gerry Chang [:gchang] from comment #61) > I've added it in 58 gdoc. Thanks!