Closed Bug 1378385 Opened 2 years ago Closed 2 years ago

Kill switch for B/C slots (background tabs bandwidth narrower)

Categories

(Core :: Networking: HTTP, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mayhemer, Assigned: kershaw)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 3 obsolete files)

The code is rather complicated, so it may not be easy to have a preference that would let us go to exactly what we had before bug 1312782.

One idea I have is to treat transactions as they were all for window id = 0 when the kill switch is 'off', i.e. put them to the '0' queue, ordered by priority.  The picking algorithm then should take 6 'background' transactions when the bandwidth is available making it work more or less as before.

If we decide it's the way, we have to wrap all queuing-logic calls to trans->TopLevelOuterContentWindowId() with a pref branching.  If we would modify TopLevelOuterContentWindowId implementations directly to return 0, it would effect throttling as well - not something I'd want.


Kershaw, what do you think?  Is the approach for the preference feasible?

pref name: network.http.active_tab_priority
Flags: needinfo?(kechang)
(In reply to Honza Bambas (:mayhemer) from comment #0)
> The code is rather complicated, so it may not be easy to have a preference
> that would let us go to exactly what we had before bug 1312782.
> 
> One idea I have is to treat transactions as they were all for window id = 0
> when the kill switch is 'off', i.e. put them to the '0' queue, ordered by
> priority.  The picking algorithm then should take 6 'background'
> transactions when the bandwidth is available making it work more or less as
> before.
> 

I think this idea is doable.
We have to
1. modify nsConnectionEntry::InsertTransaction [1] to insert all transactions into the '0' queue.
2. modify nsHttpConnectionMgr::OnMsgReschedTransaction to always use window id 0 to find the correct array.
3. modify nsHttpConnectionMgr::ProcessPendingQForEntry to only call AppendPendingQForFocusedWindow with window id 0 to append transactions into pendingQ.

[1] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/netwerk/protocol/http/nsHttpConnectionMgr.cpp#4887
[2] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/netwerk/protocol/http/nsHttpConnectionMgr.cpp#2224
[3] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/netwerk/protocol/http/nsHttpConnectionMgr.cpp#1021

> If we decide it's the way, we have to wrap all queuing-logic calls to
> trans->TopLevelOuterContentWindowId() with a pref branching.  If we would
> modify TopLevelOuterContentWindowId implementations directly to return 0, it
> would effect throttling as well - not something I'd want.
> 

With the above changes, I think we don't have to touch TopLevelOuterContentWindowId(), so throttling algorithm should be unaffected.

What do you think?
Flags: needinfo?(kechang) → needinfo?(honzab.moz)
Assignee: nobody → kechang
Status: NEW → ASSIGNED
And what if you just wrap appropriate calls to |transaction->TopLevelOuterContentWindowId()| with:

static uint64_t TabIdForQueuing(*transaction)
{
  return pref_foreground_tab_priority
    ? transaction->TopLevelOuterContentWindowId()
    : 0;
}

?

I believe case [1] and [2] would be solved, not sure about [3].  My idea was to try to pick transactions for the active tab (which we would still track) as we normally do now.  But as there were no transactions for the active tab (all transactions are in the '0' queue) we turn to picking from background queues.  There would be only one such queue - the '0' one.  Would that pick 6 transactions by priority as before the B/C slots?  If not, then you may want to wrap mCurrentTopLevelOuterContentWindowId access via a conn-manager method that would do something similar as the TabIdForQueuing function to fulfill [3].

But it's all more up to you, I'm just trying to keep the code changes simple, consistent, maintainable and readable :)

Thanks for taking this!
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-active]
Summary:
 - Add a new pref.
 - Add GetTransactionPendingQHelper for saving some duplicate code.
 - Add TabIdForQueuing to wrap TopLevelOuterContentWindowId()

Please take a look. Thanks.
Attachment #8884763 - Flags: review?(honzab.moz)
Comment on attachment 8884763 [details] [diff] [review]
Add a pref for switching off active tab priority

Review of attachment 8884763 [details] [diff] [review]:
-----------------------------------------------------------------

I would make the preference a 'restart required' to take effect (not watch it for changes).  I believe that when people play with it at runtime we may get false bug reports that stuff doesn't load (could not find transactions to dispatch).  This is for pref studies only where a run-time switching is not needed, AFAIK.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1078,2 @@
>              pendingQ,
>              maxFocusedWindowConnections);

maxFocusedWindowConnections will be 5, right?  so it will not give us the behavior as before this patch, right?

you probably need to change the maxconn calculations as well.

@@ +1112,2 @@
>              remainingPendingQ,
>              maxNonFocusedWindowConnections);

I think here it's the same problem.
Attachment #8884763 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #4)
> Comment on attachment 8884763 [details] [diff] [review]
> Add a pref for switching off active tab priority
> 
> Review of attachment 8884763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would make the preference a 'restart required' to take effect (not watch
> it for changes).  I believe that when people play with it at runtime we may
> get false bug reports that stuff doesn't load (could not find transactions
> to dispatch).  This is for pref studies only where a run-time switching is
> not needed, AFAIK.
> 

Got it.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +1078,2 @@
> >              pendingQ,
> >              maxFocusedWindowConnections);
> 
> maxFocusedWindowConnections will be 5, right?  so it will not give us the
> behavior as before this patch, right?
> 

Yes, you are tight. I've overlooked this part.
Will fix in next version.

> you probably need to change the maxconn calculations as well.
> 
> @@ +1112,2 @@
> >              remainingPendingQ,
> >              maxNonFocusedWindowConnections);
> 
> I think here it's the same problem.
Summary:
 - Add a new function PreparePendingQForDispatching() to deal with selecting transactions detail.
 - Only read the preference one time in nsHttpHandler::Init()

Thanks.
Attachment #8884763 - Attachment is obsolete: true
Attachment #8886041 - Flags: review?(honzab.moz)
Comment on attachment 8886041 [details] [diff] [review]
Add a pref for switching off active tab priority - v2

Review of attachment 8886041 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1143,5 @@
> +    nsTArray<RefPtr<PendingTransactionInfo>> pendingQ;
> +    PreparePendingQForDispatching(ent, pendingQ, considerAll);
> +
> +    if (pendingQ.IsEmpty()) {
> +        return dispatchedSuccessfully;

this bit is a bit confusing, maybe just add a comment that the pendingQ is empty when there are either no transactions to dispatch or when there are no connections avail.

@@ +2236,5 @@
> +static uint64_t TabIdForQueuing(nsAHttpTransaction *transaction)
> +{
> +  return gHttpHandler->ActiveTabPriority()
> +    ? transaction->TopLevelOuterContentWindowId()
> +    : 0;

nit: fix the indent

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +527,5 @@
> +                          bool considerAll);
> +
> +    void PreparePendingQForDispatching(nsConnectionEntry *ent,
> +                                       nsTArray<RefPtr<PendingTransactionInfo>> &pendingQ,
> +                                       bool considerAll);

a comment what this does would be nice and how the outparam pendingQ is set on various conditions

::: netwerk/test/unit/test_bug1312782_http1.js
@@ +170,5 @@
>  
>  function run_test() {
> +  var prefs = Cc["@mozilla.org/preferences-service;1"]
> +                .getService(Components.interfaces.nsIPrefBranch);
> +  prefs.setBoolPref("network.http.active_tab_priority", true);

add a comment why are you explicitly setting this pref

OTOH, would it make sense to have a new test for the case this pref is at false?  would it be easy to hg cp this test and slightly adjust it?
Attachment #8886041 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Comment on attachment 8886041 [details] [diff] [review]
> Add a pref for switching off active tab priority - v2
> 
> Review of attachment 8886041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +1143,5 @@
> > +    nsTArray<RefPtr<PendingTransactionInfo>> pendingQ;
> > +    PreparePendingQForDispatching(ent, pendingQ, considerAll);
> > +
> > +    if (pendingQ.IsEmpty()) {
> > +        return dispatchedSuccessfully;
> 
> this bit is a bit confusing, maybe just add a comment that the pendingQ is
> empty when there are either no transactions to dispatch or when there are no
> connections avail.
> 

I'll add a comment, but the only case that pendingQ is empty is when there is no connections available.
The length of pending transactions has been already checked at the beginning of ProcessPendingQForEntry().

> @@ +2236,5 @@
> > +static uint64_t TabIdForQueuing(nsAHttpTransaction *transaction)
> > +{
> > +  return gHttpHandler->ActiveTabPriority()
> > +    ? transaction->TopLevelOuterContentWindowId()
> > +    : 0;
> 
> nit: fix the indent
> 

Got it. Thanks for catching this.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +527,5 @@
> > +                          bool considerAll);
> > +
> > +    void PreparePendingQForDispatching(nsConnectionEntry *ent,
> > +                                       nsTArray<RefPtr<PendingTransactionInfo>> &pendingQ,
> > +                                       bool considerAll);
> 
> a comment what this does would be nice and how the outparam pendingQ is set
> on various conditions
> 

Will do.

> ::: netwerk/test/unit/test_bug1312782_http1.js
> @@ +170,5 @@
> >  
> >  function run_test() {
> > +  var prefs = Cc["@mozilla.org/preferences-service;1"]
> > +                .getService(Components.interfaces.nsIPrefBranch);
> > +  prefs.setBoolPref("network.http.active_tab_priority", true);
> 
> add a comment why are you explicitly setting this pref
> 
> OTOH, would it make sense to have a new test for the case this pref is at
> false?  would it be easy to hg cp this test and slightly adjust it?

So, would you like to see a test case that shows http requests are sent based on their priority?

I am using git and I am not sure if there is a command in git like hg cp. Could you accept another new test file?
Flags: needinfo?(honzab.moz)
(In reply to Kershaw Chang [:kershaw] from comment #8)
> So, would you like to see a test case that shows http requests are sent
> based on their priority?

I'm not sure what the test does, the description is super-confusing.  But I presume you know how things work when the active-tab-priority feature is off.  So, the new test to add should check we behave as before the B/C slots were implemented.

> 
> I am using git and I am not sure if there is a command in git like hg cp.
> Could you accept another new test file?

Yep.
Flags: needinfo?(honzab.moz)
Please see detailed test steps at the beginning of the file.

Thanks.
Attachment #8887340 - Flags: review?(honzab.moz)
Comment on attachment 8887340 [details] [diff] [review]
Test for "network.http.active_tab_priority" is false

Review of attachment 8887340 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  locally tested, both added tests pass, when I flip the pref in each test, they both fail.

::: netwerk/test/unit/test_bug1378385_http1.js
@@ +9,5 @@
> +// and then receive the rest 3 requests.
> +//
> +// Test step:
> +// 1. Create 6 dummy http requests. Server would not process responses until get
> +//    all 6 requests.

until told?
Attachment #8887340 - Flags: review?(honzab.moz) → review+
Rebase and carry r+.
Attachment #8886041 - Attachment is obsolete: true
Attachment #8887340 - Attachment is obsolete: true
Attachment #8887784 - Flags: review+
Carry r+.
Attachment #8887785 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/996576bdc890
Add a pref for switching off giving more priority to active tab. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac596abee3d
Test case. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/996576bdc890
https://hg.mozilla.org/mozilla-central/rev/5ac596abee3d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1383708
You need to log in before you can comment on or make changes to this bug.