We should be able to reduce the priority of network loads occurring in background pages. Open questions: * What's the right API for doing this? Is it enough to adjust the loadgroup's priority by QIing it to nsISupportsPriority? Would that API be sufficient to communicate the load priorities to an HTTP2 server, for example? * If we need to invent a new API, we should probably discuss that here. From what I can tell, nsISupportsPriority should be sufficient, but it's hard to say how much adjustment we should make when things move from being background to foreground and vice versa. * Also, how does this API deal with loads that are in progress? * How to test the patch to ensure that it's making things better?

How would this work with network requests being intercepted and passed-through by a service worker?

(In reply to Ben Kelly [:bkelly] from comment #1) > How would this work with network requests being intercepted and > passed-through by a service worker? Those shouldn't be respecting the priority, even today. (And we probably mistakenly don't account for such channels.)

For pass through requests we should be tying it back to the original document/window. That would let us handle the priority correctly. I guess we wouldn't want to deprioritize other service worker fetch requests, though. What about things like notification icons, etc. Should those be de-prioritized? Seems reasonable to delay stuff like that.

Maybe this is well known, but we already do some prioritization. http://searchfox.org/mozilla-central/source/browser/modules/NetworkPrioritizer.jsm It does go through nsISupportsPriority: http://searchfox.org/mozilla-central/source/toolkit/content/widgets/browser.xml#800 (There's a corresponding version for e10s that does the same thing.)

See also bug 1141814 ("Lower priority of HTTP requests for resources on the Tracking Protection list") and Chrome's project plan for loading priorities: https://docs.google.com/document/d/1PYIpJ9xSxoc4MHdPfPOn04HrdIRvY5B-U-SZhm0rH3Y/

one of the challenges here is that necko priortizes within origins, but not across origins. This means that a lot of the priorities that gecko sets (e.g. comment 4) don't have a lot of practical impact. The underlying challenge is that an optimal strategy depends on where the shared bottleneck is (which for shared origins is by definition at the same place). So addressing that, to at least a first order heuristic, is part of the plan too. Its worth depriortizing data transfer when it would slow other data transfer, but its less worthwhile to do so when bandwidth is available even if the cpu is working on a different tab due to the latency involved.. doing so requires buffering that will mostly just happen if an event queue isn't running to decode the ODA - backpressure will eventually kick in. Some tuning is going to be essential to find balance. The TP bits of comment 5 are in scope for the priority project, but probably need a separate bug. Additionally scheduling images based on viewport, providing boosts for latency sensitive actions, and perhaps special handling for the interaction between long downloads and short page loads - all of those are in scope but would be handled in different bugs. A project plan is still in the works we talked about it in detail last week as a necko team - honza has agreed to be the lead engineer.

(In reply to Bill McCloskey (:billm) from comment #4) > Maybe this is well known, but we already do some prioritization. > http://searchfox.org/mozilla-central/source/browser/modules/ > NetworkPrioritizer.jsm It seems like this file mostly does what this bug is filed for... See for example <http://searchfox.org/mozilla-central/source/browser/modules/NetworkPrioritizer.jsm#81>

(In reply to :Ehsan Akhgari from comment #7) > (In reply to Bill McCloskey (:billm) from comment #4) > > Maybe this is well known, but we already do some prioritization. > > http://searchfox.org/mozilla-central/source/browser/modules/ > > NetworkPrioritizer.jsm > > It seems like this file mostly does what this bug is filed for... See for > example > <http://searchfox.org/mozilla-central/source/browser/modules/ > NetworkPrioritizer.jsm#81> Yes, and it actually ends up in InsertTransactionSorted for each channel's transaction in the load group. Although, I can imagine a case where some resources on the active tab having an effect on the interaction/repaint state of the page can have the LOW priority (which is the first under NORMAL) like image channels. And those will melt with the LOW priorty of all the background stuff. We want to be smarter. If PRIORITY_DELTA was at least defined as NORMAL - LOWEST, there would be some more distinction made. But that would hit some other problems (will be stated in the CDP document I have to finish and publish.) What the content driven priority aims at is tho more "absolute" and "smarter" prioritization. We want to schedule active tab vs background tabs as something between 9:1 to 8:2. Patrick pointed out that giving total priority only to the foreground tab (+some exceptions - will be documented later) never works really well. I personally think of some kind of LRU queue, where tabs are ordered in some heuristic order of when user is going to switch to them, that we pick loadgroups from and put the 80%-90% priority on their loads. It's good to know where the priority change happens. It can be rewritten to update and notify that LRU queue for the CDP network scheduler.

Ehsan, are you OK if I hijack this bug for implementing the top level notification parts we need for more sophisticated intra-tab prioritization? There are already some good references to a code we may want to change as part of the CDP effort. See also bug 1312741 for overview.

(In reply to Honza Bambas (:mayhemer) from comment #9) > Ehsan, are you OK if I hijack this bug for implementing the top level > notification parts we need for more sophisticated intra-tab prioritization? Absolutely!

Mark P2 according to [necko-next] whiteboard

Honza, Could you please have a look at this patch? It's about adding new function in nsNetUtil.h for updating the top level window id. Not sure if it's a correct approach. Thanks.

smaug, In order to let nsHttpConnectionMgr be aware of the active tab, I've stored the current top level window id in bug 1326339. This patch is about updating the window id when the focus changes. Do you think nsFocusManager is suitable to do this? Thanks.

The title talks about active tab, and there can be several such if one has multiple windows open, but using nsFocusManager is about the top level DOM window per process.

Comment on attachment 8827139 [details] [diff] [review] Part2: Notify Necko about current top level window id So I think we don't want this approach but probably use the active state of top level content docshells

Comment on attachment 8827131 [details] [diff] [review] Part1: NS_NotifyCurrentTopLevelWindowId for updating the current top level window id Review of attachment 8827131 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: netwerk/ipc/PNecko.ipdl @@ +114,5 @@ > async RemoveRequestContext(nsCString rcid); > > async PAltDataOutputStream(nsCString type, PHttpChannel channel); > > + async NotifyCurrentTopLevelWindowId(uint64_t windowId); not sure how/where to set a priority for an IPC message, but if possible, please let this go up as a higher priority. it's a small message with a big effect :) ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +2208,5 @@ > mConnMgr->DoShiftReloadConnectionCleanup(nullptr); > } > + } else if (!strcmp(topic, "net:current-toplevel-windowid")) { > + nsCOMPtr<nsISupportsPRUint64> wrapper = do_QueryInterface(subject); > + if (wrapper) { maybe RELEASE_ASSERT(wrapper)?

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #15) > Comment on attachment 8827139 [details] [diff] [review] > Part2: Notify Necko about current top level window id > > So I think we don't want this approach but probably use the active state of > top level content docshells Thanks for the comment. Apparently, I confused the definition of the active tab. Honza, could you remind me about the goal again? Do we interest in all multiple windows in the active tab or only the current focused window? The former needs to store more than one window id in nsHttpConnectionMgr and is certainly more complicated.

(In reply to Kershaw Chang [:kershaw] from comment #17) > (In reply to Olli Pettay [:smaug] (review request backlog because of a work > week) from comment #15) > > Comment on attachment 8827139 [details] [diff] [review] > > Part2: Notify Necko about current top level window id > > > > So I think we don't want this approach but probably use the active state of > > top level content docshells > > Thanks for the comment. > Apparently, I confused the definition of the active tab. > > Honza, could you remind me about the goal again? Do we interest in all > multiple windows in the active tab or only the current focused window? The > former needs to store more than one window id in nsHttpConnectionMgr and is > certainly more complicated. Let's go simple - active tab in active window. It will solve 99% of use cases. If, from whatever reasons we decide to prioritize each window's active tab, we can well do it in a followup, if worthy...

Comment on attachment 8827139 [details] [diff] [review] Part2: Notify Necko about current top level window id Based on comment #18, I think Honza meant that we only care about the current focused top level window. smaug, do you mind having a look again? Thanks.

Comment on attachment 8827139 [details] [diff] [review] Part2: Notify Necko about current top level window id So NS_NotifyCurrentTopLevelWindowId gets called for all content windows, including iframes. I don't think that is the idea. (but it isn't clear to me what is the idea. Even if we don't care about multiple top level browser windows, a tab may contain several DOM windows because of iframes.) If we care about only top level content windows in frontmost browser window, then you should access window's .top property to get the top level window. (but does Necko understand that iframes should be treated with same priority as that window?)

(In reply to Olli Pettay [:smaug] (review request backlog because of a work week) from comment #20) > Comment on attachment 8827139 [details] [diff] [review] > Part2: Notify Necko about current top level window id > > So NS_NotifyCurrentTopLevelWindowId gets called for all content windows, > including iframes. I don't think that is the idea. (but it isn't clear to me > what is the idea. Even if we don't care about multiple top level browser > windows, a tab may contain several DOM windows because of iframes.) > > If we care about only top level content windows in frontmost browser window, > then you should access window's .top property to get the top level window. I think we only care about only the top level content window in frontmost browser window (which should be the current focused window's top?). So, I use |GetTop| to get the top level window in my patch. Perhaps I should call NS_NotifyCurrentTopLevelWindowId when the top level focused window id is really changed. > (but does Necko understand that iframes should be treated with same priority > as that window?) Yes. In bug 1326339, each http transaction stores the top level window id, so no matter those transactions coming from iframes or not will be treated in the same way. Does this sound reasonable to you?

ok, if it does have knowledge about docshell tree, then fine.

Comment on attachment 8827139 [details] [diff] [review] Part2: Notify Necko about current top level window id Then I guess this should be fine, though perhaps call NotifyCurrentTopLevelWindowChange NotifyCurrentTopLevelContentWindowChange, since it doesn't notify about chrome windows.

UpdateCurrentTopLevelWindowId() doesn't seem to be defined in either of these patches, FWIW.

See comment 13.

Summary: - Address comments in comment#16. - Change the API name to NS_NotifyCurrentTopLevelOuterContentWindowId

Comment on attachment 8843233 [details] [diff] [review] Part1: NS_NotifyCurrentTopLevelOuterContentWindowId for updating the current top level window id - v2 Review of attachment 8843233 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsNetUtil.h @@ +962,5 @@ > * pref network.http.referer.userControlPolicy > */ > uint32_t NS_GetDefaultReferrerPolicy(); > > +nsresult NS_NotifyCurrentTopLevelOuterContentWindowId(uint64_t aWindowId); add a comment what this is doing ::: netwerk/ipc/NeckoParent.cpp @@ +912,5 @@ > +mozilla::ipc::IPCResult > +NeckoParent::RecvNotifyCurrentTopLevelOuterContentWindowId(const uint64_t& aWindowId) > +{ > + if (NS_FAILED(NS_NotifyCurrentTopLevelOuterContentWindowId(aWindowId))) { > + return IPC_FAIL_NO_REASON(this); hmm... I think this will just kill the child process. we don't need to do this. I'd just ignore or top-most log any errors here and return OK. ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +2233,5 @@ > + nsCOMPtr<nsISupportsPRUint64> wrapper = do_QueryInterface(subject); > + MOZ_RELEASE_ASSERT(wrapper); > + > + uint64_t windowId = 0; > + wrapper->GetData(&windowId); can you assert windowsId != 0?

Summary: - Fix last reivew comments. - Carry reviewer's name. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3731b9402fa0a153396f2998ed06be99530da6c0

Carry reviewer's name.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/200abfdbd8ef Part1: Add new API for updating current top level outer content windowId - v2, r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/275de64c0218 Part2: Send an update of current top level, r=smaug

https://hg.mozilla.org/mozilla-central/rev/200abfdbd8ef https://hg.mozilla.org/mozilla-central/rev/275de64c0218