1312782 - Implement "B" and "C" slots in the HTTP transaction dequeue logic

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

A/B slots are described at [1].

This will probably need more bugs.  We need couple things here:

1. notification of an active tab change or navigation start (probably from [2], ideally should happen solely on the parent process) ; each channel has "root load group id" assigned [3], the notification should tell us root load group id of the active tab

when user opens a new tab, the notification must not come sooner than the navigation (load) actually starts.

2. the dequeue mechanism itself, inside the http connection manager (IIRC the code location):
- have some arbitrary limit for B + C slot (say 20, can change based on experiments)
- ensure 80-90% of that limit (say 16-18) for the B slot requests (requests of the active tab)
- use the rest for non-active tab requests

Technically, get 16-18 transactions belonging to channels with mRequestContextID == active-tab-id on wire immediately.  Fill remaining limit with anything else.  Both slots have to pick transactions according their respective priorities.

Note that when active tab changes, then everything considered before being slot "B" now becomes part of the slot "C".  Hence, that arbitrary limit will have to be disobeyed at that moment.  To explain:

* user loads foo.com, that page has a big number of sub-resources, soon we have 18 transactions receiving data (slot B is full) and number of others being queued
* user opens a new tab and goes to bar.com, we get the notification when navigation starts
* now bar.com wants to load a number of resources, but we have 18 transaction of 20 already on wire 
* we have to ignore it for now and start as if "B slot was empty", that means, put 18 transaction that bar.com load requested on wire right now (more detailed: when there is an idle connection, use it, when not, queue the transaction but also open a new connection ; now it will race - either the new connection or one of the active will become available
* since we now have 36 transactions on the wire, we may need to start throttling channels to origins from which bar.com doesn't load its resources.  this will get us more bandwidth for bar.com tab loads
* throttling can stop when the "C" slot is on reasonable number of active transactions (2-4) again, a natural converge of this algorithm

There is probably more, this is just a more detailed and more technical description of what to start with.

Of course, this all is free for discussion and also may be subject to changes when we start experiments.


[1] https://docs.google.com/document/d/1FWpLUOhR4cuXn-hB3xy6aXkDfp_aZxklG0TWTEnUU4U/edit
[2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/browser/modules/NetworkPrioritizer.jsm#81
[3] HttpBaseChannel::EnsureRequestContextID, mRequestContextID

Comment 1

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #0)
> A/B slots are described at [1].

Of course, wanted to say B/C slots :)

Comment 3

[Kershaw Chang [:kershaw]]

Honza,

If you don't mind, I would like to work on this bug.
Or, do you prefer someone else?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #3)
> Honza,
> 
> If you don't mind, I would like to work on this bug.
> Or, do you prefer someone else?

Kershaw, it's great you want to work on this.  Yes, go on!  Direct to me any questions if there is anything not clear.

Thanks.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) [off till Jan 4] from comment #0)
> A/B slots are described at [1].
> 
> This will probably need more bugs.  We need couple things here:
> 
> 1. notification of an active tab change or navigation start (probably from
> [2], ideally should happen solely on the parent process) ; each channel has
> "root load group id" assigned [3], the notification should tell us root load
> group id of the active tab
> 
> when user opens a new tab, the notification must not come sooner than the
> navigation (load) actually starts.
> 

I think the first step should be letting nsHttpConnectionMgr be aware of the current tab's window ID. Moreover, we could need to store the window ID in nsConnectionEntry so that nsHttpConnectionMgr can adjust the priority for those connection entries in the current active tab.

I'll file another bug to address this first.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Dequeue transactions according to focused window's id - v1

Summary:
 - Introduce PendingTransactionDispatcher for dispatching transactions
   Steps:
     1) Traverse all connection entries in |mCT|
     2) Move transactions from each entry's |mPendingQ| to B and C slots
     3) Dispatch transactions in B and C slots 
 - Only dispatch all transactions when |ci| is null[1]. For the other case, call |ProcessPendingQForEntry| as usual. 

[1] http://searchfox.org/mozilla-central/rev/b1aadb3572eaf7d2c70e19a2ba5413809d9ac698/netwerk/protocol/http/nsHttpConnectionMgr.cpp#2317

Honza,

Could you take a look at this patch? Thansk.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8833909 [details] [diff] [review]
Dequeue transactions according to focused window's id - v1

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

I roughly overlooked this.  thanks for the patch description, but I'd like to ask for more details on how this works (the flow of transactions, and how this exactly reacts to the window change) and why you decided to write it this way (a new class, bunch of hashtables, number of loops, etc)

I think the patch doesn't need to be so overly complicated, f- for that.  Few suggestions inlined, please feel free to oppose - with arguments :)

Please keep in mind that this will need to sync with Amy's changes in bug 1312774, that modify the same code.  Amy's patch is mostly finished, so you will probably have to base your patch on top of her patch.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2848,5 @@
> +    if ((mPendingQForFocusedWindow.Length() ==
> +            mCurrentFocusedWindowTransQueueSize) &&
> +        (mPendingQForNonFocusedWindow.Length() ==
> +            gHttpHandler->NonFocusedWindowTransQueueSize())) {
> +        return false;

I don't much follow the result here.  no comment in the header too.

@@ +2863,5 @@
> +    nsTArray<RefPtr<nsHttpTransaction>> &pendingQ)
> +{
> +    mConnectionEntryTable.Put(trans, ent);
> +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray;
> +    if (!mTransactionTable.Get(ent, &transactionArray)) {

instead of having a hashtable of [conn-entry -> pendingQ], could we just put the new queues on the conn entry class directly?  the win id of the trans is const during the trans' lifetime, so you can have a hashtable of transactions on the conn-entry like [winid -> trans] or something simpler.

@@ +2940,5 @@
> +
> +    nsresult rv;
> +    for (uint32_t i = 0; i < pendingQ.Length();) {
> +        nsHttpTransaction *trans = pendingQ[i];
> +        nsConnectionEntry *ent = mConnectionEntryTable.Get(trans);

entry can be found in the nsHttpConnectionMgr(=this)->mCT hashtable via trans->mConnInfo->HashKey()

hence, you don't need the mConnectionEntryTable hashtable?

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +675,5 @@
> +        nsTArray<RefPtr<nsHttpTransaction>> mPendingQForNonFocusedWindow;
> +        // Store the mapping of nsHttpTransaction and nsConnectionEntry
> +        nsDataHashtable<nsRefPtrHashKey<nsHttpTransaction>,
> +                        nsConnectionEntry*> mConnectionEntryTable;
> +        nsClassHashtable<nsPtrHashKey<nsConnectionEntry>,

using non-refcounted pointer as a hash key is found to be super-error-prone.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +2232,5 @@
> +                gNeckoChild->SendNotifyCurrentTopLevelWindowId(windowId);
> +            } else {
> +                static uint64_t sCurrentTopLevelWindowId = 0;
> +                if (sCurrentTopLevelWindowId != windowId) {
> +                    sCurrentTopLevelWindowId = windowId;

this probably belongs to one of the blocking bugs? ;)

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> using non-refcounted pointer as a hash key is found to be super-error-prone.

error-*prove*!

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Comment on attachment 8833909 [details] [diff] [review]
> Dequeue transactions according to focused window's id - v1
> 
> Review of attachment 8833909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I roughly overlooked this.  thanks for the patch description, but I'd like
> to ask for more details on how this works (the flow of transactions, and how
> this exactly reacts to the window change) and why you decided to write it
> this way (a new class, bunch of hashtables, number of loops, etc)
> 

First, allow me to explain why I think we need two hash tables to store the mapping of a transaction and a connection entry.

1. mConnectionEntryTable
Currently, the only way to get the correct connection entry is by using |nsHttpConnectionMgr::LookupConnectionEntry|. But at the first step of this patch, the transaction will be removed from its connection entry's pendingQ, so |nsHttpConnectionMgr::LookupConnectionEntry| could not work. Getting a connection entry from mCT with trans->mConnInfo->HashKey() directly could be wrong, because sometimes we want to get a preferred coalescing entry for this transaction. So, I think storing the mapping of a transaction and its connection entry immediately after it's been removed from pendingQ maybe the only way to do.

2. mTransactionTable
This table is mainly used in |IsConnectionEntryInUse| for telling whether a connection entry is still in use. I could also traverse mConnectionEntryTable every time to see if a connection entry is existed or not, but creating another table for saving a loop seems better.

Given that there are two tables now, encapsulating these tables in another class and providing APIs(AddTransactionFromConnectionEntry, RemoveTransaction) to maintain the tables seems to me a better way to make the code more readable.

> I think the patch doesn't need to be so overly complicated, f- for that. 
> Few suggestions inlined, please feel free to oppose - with arguments :)
> 
> Please keep in mind that this will need to sync with Amy's changes in bug
> 1312774, that modify the same code.  Amy's patch is mostly finished, so you
> will probably have to base your patch on top of her patch.
> 

I'll do this in next version.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +2848,5 @@
> > +    if ((mPendingQForFocusedWindow.Length() ==
> > +            mCurrentFocusedWindowTransQueueSize) &&
> > +        (mPendingQForNonFocusedWindow.Length() ==
> > +            gHttpHandler->NonFocusedWindowTransQueueSize())) {
> > +        return false;
> 
> I don't much follow the result here.  no comment in the header too.
> 

Returning false here means that both B and C slots are full and indicates the caller should stop calling this function.

> @@ +2863,5 @@
> > +    nsTArray<RefPtr<nsHttpTransaction>> &pendingQ)
> > +{
> > +    mConnectionEntryTable.Put(trans, ent);
> > +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray;
> > +    if (!mTransactionTable.Get(ent, &transactionArray)) {
> 
> instead of having a hashtable of [conn-entry -> pendingQ], could we just put
> the new queues on the conn entry class directly?  the win id of the trans is
> const during the trans' lifetime, so you can have a hashtable of
> transactions on the conn-entry like [winid -> trans] or something simpler.
> 

As I explained above, |mTransactionTable| is just for querying if a connection entry is in use in PendingTransactionDispatcher. For this purpose, I think we don't have to put another queue in connection entry.

> @@ +2940,5 @@
> > +
> > +    nsresult rv;
> > +    for (uint32_t i = 0; i < pendingQ.Length();) {
> > +        nsHttpTransaction *trans = pendingQ[i];
> > +        nsConnectionEntry *ent = mConnectionEntryTable.Get(trans);
> 
> entry can be found in the nsHttpConnectionMgr(=this)->mCT hashtable via
> trans->mConnInfo->HashKey()
> 
> hence, you don't need the mConnectionEntryTable hashtable?
> 

Due to spdy coalescing, we cannot always get a connection entry by mConnInfo->HashKey(). I think we still need a table to save the mapping.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +675,5 @@
> > +        nsTArray<RefPtr<nsHttpTransaction>> mPendingQForNonFocusedWindow;
> > +        // Store the mapping of nsHttpTransaction and nsConnectionEntry
> > +        nsDataHashtable<nsRefPtrHashKey<nsHttpTransaction>,
> > +                        nsConnectionEntry*> mConnectionEntryTable;
> > +        nsClassHashtable<nsPtrHashKey<nsConnectionEntry>,
> 
> using non-refcounted pointer as a hash key is found to be super-error-prone.
> 

Unfortunately, nsConnectionEntry is not ref-counted. However, since nsConnectionEntry is only used inside http connection manager, I think it should be ok if I handle this with caution?

> ::: netwerk/protocol/http/nsHttpHandler.cpp
> @@ +2232,5 @@
> > +                gNeckoChild->SendNotifyCurrentTopLevelWindowId(windowId);
> > +            } else {
> > +                static uint64_t sCurrentTopLevelWindowId = 0;
> > +                if (sCurrentTopLevelWindowId != windowId) {
> > +                    sCurrentTopLevelWindowId = windowId;
> 
> this probably belongs to one of the blocking bugs? ;)

Yes, I'll move this part away.

Comment 10

[Honza Bambas (:mayhemer)]

comment 9

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #9)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > Comment on attachment 8833909 [details] [diff] [review]
> > Dequeue transactions according to focused window's id - v1
> > 
> > Review of attachment 8833909 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I roughly overlooked this.  thanks for the patch description, but I'd like
> > to ask for more details on how this works (the flow of transactions, and how
> > this exactly reacts to the window change) and why you decided to write it
> > this way (a new class, bunch of hashtables, number of loops, etc)
> > 
> 
> First, allow me to explain why I think we need two hash tables to store the
> mapping of a transaction and a connection entry.
> 
> 1. mConnectionEntryTable
> Currently, the only way to get the correct connection entry is by using
> |nsHttpConnectionMgr::LookupConnectionEntry|. But at the first step of this
> patch, the transaction will be removed from its connection entry's pendingQ,

Don't remove it then? :D

> so |nsHttpConnectionMgr::LookupConnectionEntry| could not work. Getting a
> connection entry from mCT with trans->mConnInfo->HashKey() directly could be
> wrong, because sometimes we want to get a preferred coalescing entry for
> this transaction. 

Aha.  So, we should have a trans->mPreferredConnInfo then.  But I'm not sure how exactly to deal with this.  The first version of the patch might even try to ignore coalescing, if possible.  We'll see.

> So, I think storing the mapping of a transaction and its
> connection entry immediately after it's been removed from pendingQ maybe the
> only way to do.

And I still don't think so.

> 
> 2. mTransactionTable
> This table is mainly used in |IsConnectionEntryInUse| for telling whether a
> connection entry is still in use. I could also traverse
> mConnectionEntryTable every time to see if a connection entry is existed or
> not, but creating another table for saving a loop seems better.

Please have a new member on connection entry class to keep this data for you, if needed.  You really don't need a hashtable somewhere else for this.  But first please read my longer comment below these notes.  

> 
> Given that there are two tables now, encapsulating these tables in another
> class and providing APIs(AddTransactionFromConnectionEntry,
> RemoveTransaction) to maintain the tables seems to me a better way to make
> the code more readable.

Hmm.. not really.  I still think this is too complicated.  But let me go on..

> As I explained above, |mTransactionTable| is just for querying if a
> connection entry is in use in PendingTransactionDispatcher. For this
> purpose, I think we don't have to put another queue in connection entry.

Hmm.. "checking if in use" via a hash table.  That sounds odd to me, sorry.  What exactly means "is in use?"

> Due to spdy coalescing, we cannot always get a connection entry by
> mConnInfo->HashKey(). I think we still need a table to save the mapping.

Yup.

> Unfortunately, nsConnectionEntry is not ref-counted. However, since
> nsConnectionEntry is only used inside http connection manager, I think it
> should be ok if I handle this with caution?

Absolutely not!  if the instance goes away and you don't remove it from your hashtable keys, you will have a wrong pointer as a key!  and when the pointer gets recycled by the allocator (happens pretty often) your hashtable key will point at a totally different connection entry or a totaly different object!  that is unacceptable way of architecting a code, sorry.  we have to rethink this.




So, let's step back a bit now from how to implement this feature and rather think of it more schematically, more about what we actually want here.

This feature will have way different logic for HTTP/1 and HTTP/2.

Let's start with HTTP/1.  There is no host coalescing.  Each host has a (preferable) limit of 6 parallel connections.  What I want is to use (say) 5 connections to a host for serving requests for the active tab and one connection for this host to serve (best the highest priority) for a request coming from a background tab (if there is one, otherwise of course use this sixth connection also for the active tab).

So, you will pick from the ci pending q transactions only those for which the trans->winid == connman->activewinid until you fill the limit of (say) 5.  Suitable may be a hashtable <winid : transactions-by-priority[]> on connection entry (side by the regular pending q) to implement this.  You will simply pick 5 transactions from this hashtable by conn-man->activewinid as the key and then pick a random (or chosen with some smarter logic!) array from this hashtable to utilize the sixth connection (if there are transactions pending for different tabs, of course).  Picking for the C slot is trickier than picking for the B slot, yes.  The patch we land here doesn't need to be perfect at the first stage.  But I think the hashtable <winid : transactions-by-priority[]> on connection entry is an obvious step that should ensure the field is prepared for any followups.

When a tab switches, you will simply be picking from your hashtable using the new winid (for the 5 connections, aka the B slot).  transactions already running for the now inactive window will be left to finish.  here we can optimize (in followups, please!) with opening more connections, over the limit of 6, throttling the running background tab transactions or so.  These optimization shall be done in followups incrementally.



And now HTTP/2.  There is host coalescing.  I personally don't know how exactly h2 coalescing and per host limiting works.  I'll ask Patrick personally.  I'm not so sure coalescing is any obstacle we should deal with or be bothered with.  Let's try discussing this w/o host coalescing first.  You need to realize we have far greater parallelism limit than with H1.  Here you can push 100 transactions on a single http2 session as they are requested.  What you want to do here is to change priorities of transaction grouped by the win-id.  The priority of transactions for the active tab should be raised (not sure by what factor, more later about it).  When the active tab changes, raise priority of the newly active tab transactions and drop priority of previously active tab transactions (by the same factor) ; preferably in this order.  The inc/dec factor might be either higher, so that active tab's transactions will beat all transactions from other (inactive) tabs or can be just marginal, so that there will be an overlap: high priority transactions from background tabs will then have priority a bit higher than the lowest priority transactions for the active tab.  This is something to be tested during actual usage of the browser, so best would be to make the factor preferable.

And again, a hashtable <winid : transactions-by-priority[]> on connection entry is something to use here, right?

What do you think?

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> (In reply to Kershaw Chang [:kershaw] from comment #9)
> > (In reply to Honza Bambas (:mayhemer) from comment #7)
> > > Comment on attachment 8833909 [details] [diff] [review]
> > > Dequeue transactions according to focused window's id - v1
> > > 
> > > Review of attachment 8833909 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > I roughly overlooked this.  thanks for the patch description, but I'd like
> > > to ask for more details on how this works (the flow of transactions, and how
> > > this exactly reacts to the window change) and why you decided to write it
> > > this way (a new class, bunch of hashtables, number of loops, etc)
> > > 
> > 
> > First, allow me to explain why I think we need two hash tables to store the
> > mapping of a transaction and a connection entry.
> > 
> > 1. mConnectionEntryTable
> > Currently, the only way to get the correct connection entry is by using
> > |nsHttpConnectionMgr::LookupConnectionEntry|. But at the first step of this
> > patch, the transaction will be removed from its connection entry's pendingQ,
> 
> Don't remove it then? :D
> 
> > so |nsHttpConnectionMgr::LookupConnectionEntry| could not work. Getting a
> > connection entry from mCT with trans->mConnInfo->HashKey() directly could be
> > wrong, because sometimes we want to get a preferred coalescing entry for
> > this transaction. 
> 
> Aha.  So, we should have a trans->mPreferredConnInfo then.  But I'm not sure
> how exactly to deal with this.  The first version of the patch might even
> try to ignore coalescing, if possible.  We'll see.
> 
> > So, I think storing the mapping of a transaction and its
> > connection entry immediately after it's been removed from pendingQ maybe the
> > only way to do.
> 
> And I still don't think so.
> 
> > 
> > 2. mTransactionTable
> > This table is mainly used in |IsConnectionEntryInUse| for telling whether a
> > connection entry is still in use. I could also traverse
> > mConnectionEntryTable every time to see if a connection entry is existed or
> > not, but creating another table for saving a loop seems better.
> 
> Please have a new member on connection entry class to keep this data for
> you, if needed.  You really don't need a hashtable somewhere else for this. 
> But first please read my longer comment below these notes.  
> 
> > 
> > Given that there are two tables now, encapsulating these tables in another
> > class and providing APIs(AddTransactionFromConnectionEntry,
> > RemoveTransaction) to maintain the tables seems to me a better way to make
> > the code more readable.
> 
> Hmm.. not really.  I still think this is too complicated.  But let me go on..
> 
> > As I explained above, |mTransactionTable| is just for querying if a
> > connection entry is in use in PendingTransactionDispatcher. For this
> > purpose, I think we don't have to put another queue in connection entry.
> 
> Hmm.. "checking if in use" via a hash table.  That sounds odd to me, sorry. 
> What exactly means "is in use?"
> 
> > Due to spdy coalescing, we cannot always get a connection entry by
> > mConnInfo->HashKey(). I think we still need a table to save the mapping.
> 
> Yup.
> 
> > Unfortunately, nsConnectionEntry is not ref-counted. However, since
> > nsConnectionEntry is only used inside http connection manager, I think it
> > should be ok if I handle this with caution?
> 
> Absolutely not!  if the instance goes away and you don't remove it from your
> hashtable keys, you will have a wrong pointer as a key!  and when the
> pointer gets recycled by the allocator (happens pretty often) your hashtable
> key will point at a totally different connection entry or a totaly different
> object!  that is unacceptable way of architecting a code, sorry.  we have to
> rethink this.
> 
> 
> 
> 
> So, let's step back a bit now from how to implement this feature and rather
> think of it more schematically, more about what we actually want here.
> 
> This feature will have way different logic for HTTP/1 and HTTP/2.
> 
> Let's start with HTTP/1.  There is no host coalescing.  Each host has a
> (preferable) limit of 6 parallel connections.  What I want is to use (say) 5
> connections to a host for serving requests for the active tab and one
> connection for this host to serve (best the highest priority) for a request
> coming from a background tab (if there is one, otherwise of course use this
> sixth connection also for the active tab).
> 
> So, you will pick from the ci pending q transactions only those for which
> the trans->winid == connman->activewinid until you fill the limit of (say)
> 5.  Suitable may be a hashtable <winid : transactions-by-priority[]> on
> connection entry (side by the regular pending q) to implement this.  You
> will simply pick 5 transactions from this hashtable by conn-man->activewinid
> as the key and then pick a random (or chosen with some smarter logic!) array
> from this hashtable to utilize the sixth connection (if there are
> transactions pending for different tabs, of course).  Picking for the C slot
> is trickier than picking for the B slot, yes.  The patch we land here
> doesn't need to be perfect at the first stage.  But I think the hashtable
> <winid : transactions-by-priority[]> on connection entry is an obvious step
> that should ensure the field is prepared for any followups.
> 
> When a tab switches, you will simply be picking from your hashtable using
> the new winid (for the 5 connections, aka the B slot).  transactions already
> running for the now inactive window will be left to finish.  here we can
> optimize (in followups, please!) with opening more connections, over the
> limit of 6, throttling the running background tab transactions or so.  These
> optimization shall be done in followups incrementally.
> 
> 
> 
> And now HTTP/2.  There is host coalescing.  I personally don't know how
> exactly h2 coalescing and per host limiting works.  I'll ask Patrick
> personally.  I'm not so sure coalescing is any obstacle we should deal with
> or be bothered with.  Let's try discussing this w/o host coalescing first. 
> You need to realize we have far greater parallelism limit than with H1. 
> Here you can push 100 transactions on a single http2 session as they are
> requested.  What you want to do here is to change priorities of transaction
> grouped by the win-id.  The priority of transactions for the active tab
> should be raised (not sure by what factor, more later about it).  When the
> active tab changes, raise priority of the newly active tab transactions and
> drop priority of previously active tab transactions (by the same factor) ;
> preferably in this order.  The inc/dec factor might be either higher, so
> that active tab's transactions will beat all transactions from other
> (inactive) tabs or can be just marginal, so that there will be an overlap:
> high priority transactions from background tabs will then have priority a
> bit higher than the lowest priority transactions for the active tab.  This
> is something to be tested during actual usage of the browser, so best would
> be to make the factor preferable.
> 
> And again, a hashtable <winid : transactions-by-priority[]> on connection
> entry is something to use here, right?
> 
> What do you think?

I think I got your point. I'll start implementing right away.
Keep you updated if I encounter any problem. Thanks.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #12)
> I think I got your point. I'll start implementing right away.
> Keep you updated if I encounter any problem. Thanks.

Feel free to put here any wip patches and ask for feedback, at any time.  Try to keep the patch in this bug simple, think of it more as a base that we can then add on to in followup bugs incrementally.

Thanks!

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: WIP

Hi Honza,

Could you take a look at this WIP patch?
This patch only implements HTTP/1 part.

Thanks.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: WIP

Update the correct patch.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8837580 [details] [diff] [review]
WIP

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

yes!  looks good.  note again that you will need to rebase your patch on top of the patch from bug 1312774, amy is touching pretty much the same area as you do.  her patch is mostly done, few nits to fix only.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +944,5 @@
> +    pendingQ.Clear();
> +
> +    // XXX currently maxCount is hard coded to 1.
> +    ent->GetRestPendingTransactions(mCurrentTopLevelWindowId, pendingQ, 1);
> +    dispatchedSuccessfully |= DoProcessPendingQ(ent, pendingQ, considerAll);

so, here if you don't dispatch, you can use the sixth (in general - any available) parallel conn(s) for the active win, right?

@@ +2273,5 @@
> +        if (windowId) {
> +            nsTArray<RefPtr<nsHttpTransaction>> *transactions =
> +                ent->mPendingTransactionTable.Get(windowId);
> +            int32_t index = transactions->IndexOf(trans);
> +            if (index >= 0) {

this should almost be an assertion and not a condition

@@ +2306,5 @@
>              LookupConnectionEntry(trans->ConnectionInfo(), nullptr, trans);
>  
>          if (ent) {
> +            nsTArray<RefPtr<nsHttpTransaction>> *transactionArray;
> +            if (ent->mPendingTransactionTable.Get(trans->TopLevelOuterWindowId(),

yeah, so on the concrete class the id is exposed.

I'm not sure how complicated would be to move TopLevelOuterWindowId to the AHttpTrans abstract class.  And if available on all derivatives.  There are six classes derived from it, you would need to check if all derivatives can be bound to a win-id or can be more general purpose.  This might become tricky, but for performance sake I would put some effort into it.

@@ +2413,5 @@
> +            nsHttpTransaction *trans = (*it.UserData())[0];
> +            LOG(("nsHttpConnectionMgr::OnMsgCancelTransactions %s %p %p\n",
> +                 ci->HashKey().get(), ent, trans));
> +            trans->Close(reason);
> +            it.UserData()->RemoveElementAt(0);

suboptimal: makes the array content to be memmoved ; better remove from the end or just clear the array when you are done with it here.

@@ +2419,2 @@
>      }
> +    ent->mPendingTransactionTable.Clear();

this may be enough to drop/clear all the arrays.

@@ +4052,5 @@
> +nsHttpConnectionMgr::
> +nsConnectionEntry::GetWindowIdByTransaction(nsAHttpTransaction *trans) const
> +{
> +  for (auto it = mPendingTransactionTable.ConstIter(); !it.Done(); it.Next()) {
> +    if (it.UserData()->Contains(trans)) {

this is super-expensive.  can we expose win-id on AHttpTransaction somehow?

@@ +4120,5 @@
> +        }
> +
> +        while (it.UserData()->Length()) {
> +            InsertTransactionSorted(result,
> +                                    it.UserData()->ElementAt(0));

isn't it already sorted in the array you keep in the hashtable?

@@ +4121,5 @@
> +
> +        while (it.UserData()->Length()) {
> +            InsertTransactionSorted(result,
> +                                    it.UserData()->ElementAt(0));
> +            it.UserData()->RemoveElementAt(0);

this memmoves the whole array.  could you rather iterate and finally removeelements or something a bit more optimal?

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +395,5 @@
> +
> +        // Return the count of pending transactions
> +        uint32_t PendingQLength() const;
> +
> +        // Return the window id of the transaction. Return 0 if not found.

not found where?  or, what is it not found?

Comment 17

[Kershaw Chang [:kershaw]]

Thanks for the feedback.

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 8837580 [details] [diff] [review]
> WIP
> 
> Review of attachment 8837580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> yes!  looks good.  note again that you will need to rebase your patch on top
> of the patch from bug 1312774, amy is touching pretty much the same area as
> you do.  her patch is mostly done, few nits to fix only.
> 

Ok, will rebase on her patch in next version.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +944,5 @@
> > +    pendingQ.Clear();
> > +
> > +    // XXX currently maxCount is hard coded to 1.
> > +    ent->GetRestPendingTransactions(mCurrentTopLevelWindowId, pendingQ, 1);
> > +    dispatchedSuccessfully |= DoProcessPendingQ(ent, pendingQ, considerAll);
> 
> so, here if you don't dispatch, you can use the sixth (in general - any
> available) parallel conn(s) for the active win, right?
> 

I am not sure what exactly I should do here.
I guess we have to count how many connections are allowed to create (mMaxPersistConnsPerHost - (ent->mActiveConns.Length() + ent->UnconnectedHalfOpens())) before retrieving transactions from pending queue?

For example, ent A has 2 idle connections, 2 active connections, and 2 unconnected half socket now. So, we can only dequeue 2 transactions for dispatching?

> @@ +2273,5 @@
> > +        if (windowId) {
> > +            nsTArray<RefPtr<nsHttpTransaction>> *transactions =
> > +                ent->mPendingTransactionTable.Get(windowId);
> > +            int32_t index = transactions->IndexOf(trans);
> > +            if (index >= 0) {
> 
> this should almost be an assertion and not a condition
> 

OK.

> @@ +2306,5 @@
> >              LookupConnectionEntry(trans->ConnectionInfo(), nullptr, trans);
> >  
> >          if (ent) {
> > +            nsTArray<RefPtr<nsHttpTransaction>> *transactionArray;
> > +            if (ent->mPendingTransactionTable.Get(trans->TopLevelOuterWindowId(),
> 
> yeah, so on the concrete class the id is exposed.
> 
> I'm not sure how complicated would be to move TopLevelOuterWindowId to the
> AHttpTrans abstract class.  And if available on all derivatives.  There are
> six classes derived from it, you would need to check if all derivatives can
> be bound to a win-id or can be more general purpose.  This might become
> tricky, but for performance sake I would put some effort into it.
> 

How about adding |virtual uint64_t TopLevelOuterWindowId() { return 0; }| in nsAHttpTransaction and only override this function in nsHttpTransaction?

> @@ +2413,5 @@
> > +            nsHttpTransaction *trans = (*it.UserData())[0];
> > +            LOG(("nsHttpConnectionMgr::OnMsgCancelTransactions %s %p %p\n",
> > +                 ci->HashKey().get(), ent, trans));
> > +            trans->Close(reason);
> > +            it.UserData()->RemoveElementAt(0);
> 
> suboptimal: makes the array content to be memmoved ; better remove from the
> end or just clear the array when you are done with it here.
> 

Good idea.

> @@ +2419,2 @@
> >      }
> > +    ent->mPendingTransactionTable.Clear();
> 
> this may be enough to drop/clear all the arrays.
> 
> @@ +4052,5 @@
> > +nsHttpConnectionMgr::
> > +nsConnectionEntry::GetWindowIdByTransaction(nsAHttpTransaction *trans) const
> > +{
> > +  for (auto it = mPendingTransactionTable.ConstIter(); !it.Done(); it.Next()) {
> > +    if (it.UserData()->Contains(trans)) {
> 
> this is super-expensive.  can we expose win-id on AHttpTransaction somehow?
> 

Yes, I think I can do it.

> @@ +4120,5 @@
> > +        }
> > +
> > +        while (it.UserData()->Length()) {
> > +            InsertTransactionSorted(result,
> > +                                    it.UserData()->ElementAt(0));
> 
> isn't it already sorted in the array you keep in the hashtable?
> 

Yes, it's already sorted. But, the elements in |result| could be retrieved from multiple pending queues. I have to make sure the priority order in |result| is correct.

> @@ +4121,5 @@
> > +
> > +        while (it.UserData()->Length()) {
> > +            InsertTransactionSorted(result,
> > +                                    it.UserData()->ElementAt(0));
> > +            it.UserData()->RemoveElementAt(0);
> 
> this memmoves the whole array.  could you rather iterate and finally
> removeelements or something a bit more optimal?
> 

I'll try to improve at next version.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +395,5 @@
> > +
> > +        // Return the count of pending transactions
> > +        uint32_t PendingQLength() const;
> > +
> > +        // Return the window id of the transaction. Return 0 if not found.
> 
> not found where?  or, what is it not found?

This function can be removed if we add |TopLevelOuterWindowId| in nsAHttpTransaction.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #17)
> > so, here if you don't dispatch, you can use the sixth (in general - any
> > available) parallel conn(s) for the active win, right?
> > 
> 
> I am not sure what exactly I should do here.
> I guess we have to count how many connections are allowed to create
> (mMaxPersistConnsPerHost - (ent->mActiveConns.Length() +
> ent->UnconnectedHalfOpens())) before retrieving transactions from pending
> queue?
> 
> For example, ent A has 2 idle connections, 2 active connections, and 2
> unconnected half socket now. So, we can only dequeue 2 transactions for
> dispatching?

Yes, of course.  What I wanted to say was: when there is no other-than-active-window-id transaction, just use/open any remaining connections up to the limit per host for the active-window-id transactions.  W/o it you will utilize only 5 connections out of 6 we allow.  If there are non-active transactions, you will use 1 for those.  If there are none, you will not utilize the whole allowed parallelism.

> How about adding |virtual uint64_t TopLevelOuterWindowId() { return 0; }| in
> nsAHttpTransaction and only override this function in nsHttpTransaction?

If that's enough for making this code work, then yes.  But I believe at least nulltransaction should implement it too.

Might be interesting to have nsAHttpTransaction::TopLevelOuterWindowId() { MOZ_ASSERT(false); return 0; } so that we catch missing implementations in subclasses.  Just an idea..

> > isn't it already sorted in the array you keep in the hashtable?
> > 
> 
> Yes, it's already sorted. But, the elements in |result| could be retrieved
> from multiple pending queues. I have to make sure the priority order in
> |result| is correct.

Aha, then please add a comment why you "sort again".

> > > +        // Return the window id of the transaction. Return 0 if not found.
> > 
> > not found where?  or, what is it not found?
> 
> This function can be removed if we add |TopLevelOuterWindowId| in
> nsAHttpTransaction.

Yup

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: patch - v2

Honza, please have a look. Thanks.

Summary of change:
1. Rebase on bug 1312774
2. Address previous feedback.

TODO:
1. For HTTP2, increase the priority of transactions from focused window.
2. A better way to dequeue transactions from non-focused window.
3. Handle the case when the focused window is changed.

I think the above items will be discussed and implemented in another followup bugs? Is there anything I missed or you think should be included inside the scope of this bug?

Thanks.

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8839443 [details] [diff] [review]
patch - v2

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

The patch unfortunately doesn't apply over the latest Amy's patch so I cannot test it :(

you should wait with merging, since Amy has to update the patch once more over a different patch in this code.

I wanted to give r+ but I don't see a try run nor I could test the patch manually (didn't apply).  So, I'd rather have a working and tested version to give an r+ to.

::: netwerk/protocol/http/ConnectionDiagnostics.cpp
@@ +84,5 @@
>      }
> +    i = 0;
> +    for (auto it = ent->mPendingTransactionTable.Iter();
> +         !it.Done();
> +         it.Next()) {

log here the win id?

::: netwerk/protocol/http/NullHttpTransaction.h
@@ +48,5 @@
>    {
>      return PR_SecondsToInterval(15);
>    }
>  
> +  uint64_t TopLevelOuterWindowId() override { return 0; }

I think nulltransaction is never put to any queue, so this should be alright.  what happens when you leave the default impl that asserts?

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +550,5 @@
>          if (ent->mIdleConns.Length()    == 0 &&
>              ent->mActiveConns.Length()  == 0 &&
>              ent->mHalfOpens.Length()    == 0 &&
>              ent->mUrgentStartQ.Length() == 0 &&
> +            ent->PendingQLength()     == 0) {

nit: keep the == 0 lined up

@@ +979,2 @@
>  
> +    uint32_t countForNewConnection = AvailableCountForNewConnection(ent);

"countFor" sounds odd, can you try finding a better name scheme here?  Like newConnectionAvailableCount or so.  The function name would use some change too.

@@ +985,5 @@
> +
> +    uint32_t countForFocusedWindow =
> +        countForNewConnection * gHttpHandler->FocusedWindowTransactionRatio();
> +    if (countForFocusedWindow > countForNewConnection) {
> +        MOZ_ASSERT(false, "Wrong pref for focused_window_transaction_ratio");

you can clip the value when you read the pref

@@ +1033,5 @@
> +         focusedWindowTrans.Length(), nonFocusedWindowTrans.Length()));
> +
> +    dispatchedSuccessfully |=
> +        DispatchPendingQ(focusedWindowTrans, ent, considerAll);
> +    // Put the leftover into connection entry

blank line before this line please

@@ +1205,5 @@
>          LOG(("  num active conns == max conns\n"));
>          return true;
>      }
>  
> +    bool result = !(AvailableCountForNewConnection(ent) > 0);

just !AvailableCountForNewConnection(ent) ?

@@ +2198,5 @@
> +    // XXX Get all transactions for SPDY currently.
> +    ent->GetRestPendingTransactions(0, pendingQ, 0);
> +    DispatchSpdyPendingQ(pendingQ, ent, conn);
> +
> +    // Put the left transactions back in the pending queue.

"the _left_ transactions" ?  should be leftover?

@@ +2532,5 @@
> +static void
> +CancelTransactionsHelper(nsTArray<RefPtr<nsHttpTransaction>> &transactions,
> +                         nsresult reason)
> +{
> +    for (auto trans : transactions) {

a bit cheaper than auto is to use the end-class ptr type (to save addref/release).  here the object is held by the array, so it's safe.  there are more places in this patch, up to you to fix it or leave it.

@@ +4223,5 @@
> +    uint64_t currentTopLevelWindowId,
> +    nsTArray<RefPtr<nsHttpTransaction>> &result,
> +    uint32_t maxCount)
> +{
> +    // Put the transactions from the focused window in the front of the quque.

quque

@@ +4226,5 @@
> +{
> +    // Put the transactions from the focused window in the front of the quque.
> +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
> +    if (!mPendingTransactionTable.Get(currentTopLevelWindowId, &transactionArray)) {
> +        return;

should we result.Clear() before return?

@@ +4230,5 @@
> +        return;
> +    }
> +
> +    uint32_t count = 0;
> +    for (; count < transactionArray->Length(); count++) {

++count may be cheaper

@@ +4234,5 @@
> +    for (; count < transactionArray->Length(); count++) {
> +        if (maxCount && count == maxCount) {
> +            break;
> +        }
> +        result.AppendElement(transactionArray->ElementAt(count));

note that you have a more efficient combination of nsTArray::InsertElementsAt and ReplaceElementsAt.  worth looking at those two.  this is hot path code, should be well optimized.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +541,4 @@
>                                bool considerAll);
>      bool     IsUnderPressure(nsConnectionEntry *ent,
>                               nsHttpTransaction::Classifier classification);
> +    uint32_t AvailableCountForNewConnection(nsConnectionEntry * ent);

please add a comment what this is doing or change the name to something more explanatory.

Comment 21

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> Comment on attachment 8839443 [details] [diff] [review]
> patch - v2
> 
> Review of attachment 8839443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch unfortunately doesn't apply over the latest Amy's patch so I
> cannot test it :(
> 

I am sorry about that. Next time I'll make sure this patch can be applied on latest Amy's patch.

> you should wait with merging, since Amy has to update the patch once more
> over a different patch in this code.
> 

It seems that Amy still has some rebase to do. I'll try to land bug 1309653 and bug 1326339 first, and then come back to this bug.

> I wanted to give r+ but I don't see a try run nor I could test the patch
> manually (didn't apply).  So, I'd rather have a working and tested version
> to give an r+ to.
> 
> ::: netwerk/protocol/http/ConnectionDiagnostics.cpp
> @@ +84,5 @@
> >      }
> > +    i = 0;
> > +    for (auto it = ent->mPendingTransactionTable.Iter();
> > +         !it.Done();
> > +         it.Next()) {
> 
> log here the win id?
> 

Will do.

> ::: netwerk/protocol/http/NullHttpTransaction.h
> @@ +48,5 @@
> >    {
> >      return PR_SecondsToInterval(15);
> >    }
> >  
> > +  uint64_t TopLevelOuterWindowId() override { return 0; }
> 
> I think nulltransaction is never put to any queue, so this should be
> alright.  what happens when you leave the default impl that asserts?
> 

Not sure what you mean here. Do you mean if there is another child class needed to also override |TopLevelOuterWindowId|?

I think we only have to override |TopLevelOuterWindowId| for nsHttpTransaction and NullHttpTransaction.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +550,5 @@
> >          if (ent->mIdleConns.Length()    == 0 &&
> >              ent->mActiveConns.Length()  == 0 &&
> >              ent->mHalfOpens.Length()    == 0 &&
> >              ent->mUrgentStartQ.Length() == 0 &&
> > +            ent->PendingQLength()     == 0) {
> 
> nit: keep the == 0 lined up
> 

Sure.

> @@ +979,2 @@
> >  
> > +    uint32_t countForNewConnection = AvailableCountForNewConnection(ent);
> 
> "countFor" sounds odd, can you try finding a better name scheme here?  Like
> newConnectionAvailableCount or so.  The function name would use some change
> too.
> 

I'll use newConnectionAvailableCount as you suggested.

> @@ +985,5 @@
> > +
> > +    uint32_t countForFocusedWindow =
> > +        countForNewConnection * gHttpHandler->FocusedWindowTransactionRatio();
> > +    if (countForFocusedWindow > countForNewConnection) {
> > +        MOZ_ASSERT(false, "Wrong pref for focused_window_transaction_ratio");
> 
> you can clip the value when you read the pref
> 

Ok.

> @@ +1033,5 @@
> > +         focusedWindowTrans.Length(), nonFocusedWindowTrans.Length()));
> > +
> > +    dispatchedSuccessfully |=
> > +        DispatchPendingQ(focusedWindowTrans, ent, considerAll);
> > +    // Put the leftover into connection entry
> 
> blank line before this line please
> 

Will do.

> @@ +1205,5 @@
> >          LOG(("  num active conns == max conns\n"));
> >          return true;
> >      }
> >  
> > +    bool result = !(AvailableCountForNewConnection(ent) > 0);
> 
> just !AvailableCountForNewConnection(ent) ?
> 

Sure.

> @@ +2198,5 @@
> > +    // XXX Get all transactions for SPDY currently.
> > +    ent->GetRestPendingTransactions(0, pendingQ, 0);
> > +    DispatchSpdyPendingQ(pendingQ, ent, conn);
> > +
> > +    // Put the left transactions back in the pending queue.
> 
> "the _left_ transactions" ?  should be leftover?
> 

Yes, you are right. Please forgive my poor English.

> @@ +2532,5 @@
> > +static void
> > +CancelTransactionsHelper(nsTArray<RefPtr<nsHttpTransaction>> &transactions,
> > +                         nsresult reason)
> > +{
> > +    for (auto trans : transactions) {
> 
> a bit cheaper than auto is to use the end-class ptr type (to save
> addref/release).  here the object is held by the array, so it's safe.  there
> are more places in this patch, up to you to fix it or leave it.
> 

I'll fix it.

> @@ +4223,5 @@
> > +    uint64_t currentTopLevelWindowId,
> > +    nsTArray<RefPtr<nsHttpTransaction>> &result,
> > +    uint32_t maxCount)
> > +{
> > +    // Put the transactions from the focused window in the front of the quque.
> 
> quque
> 

Got it.

> @@ +4226,5 @@
> > +{
> > +    // Put the transactions from the focused window in the front of the quque.
> > +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
> > +    if (!mPendingTransactionTable.Get(currentTopLevelWindowId, &transactionArray)) {
> > +        return;
> 
> should we result.Clear() before return?
> 

I'd rather not do it. Since |GetPendingQForFocusedWindow| only append transactions in result and not clear the original elements in result, it would be better not to clear result for consistency. 

> @@ +4230,5 @@
> > +        return;
> > +    }
> > +
> > +    uint32_t count = 0;
> > +    for (; count < transactionArray->Length(); count++) {
> 
> ++count may be cheaper
> 

Ok.

> @@ +4234,5 @@
> > +    for (; count < transactionArray->Length(); count++) {
> > +        if (maxCount && count == maxCount) {
> > +            break;
> > +        }
> > +        result.AppendElement(transactionArray->ElementAt(count));
> 
> note that you have a more efficient combination of
> nsTArray::InsertElementsAt and ReplaceElementsAt.  worth looking at those
> two.  this is hot path code, should be well optimized.
> 

Thanks. I'll try to improve here.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +541,4 @@
> >                                bool considerAll);
> >      bool     IsUnderPressure(nsConnectionEntry *ent,
> >                               nsHttpTransaction::Classifier classification);
> > +    uint32_t AvailableCountForNewConnection(nsConnectionEntry * ent);
> 
> please add a comment what this is doing or change the name to something more
> explanatory.

Will do.

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #21)
> > ::: netwerk/protocol/http/NullHttpTransaction.h
> > @@ +48,5 @@
> > >    {
> > >      return PR_SecondsToInterval(15);
> > >    }
> > >  
> > > +  uint64_t TopLevelOuterWindowId() override { return 0; }
> > 
> > I think nulltransaction is never put to any queue, so this should be
> > alright.  what happens when you leave the default impl that asserts?
> > 
> 
> Not sure what you mean here. Do you mean if there is another child class
> needed to also override |TopLevelOuterWindowId|?

No, I mean to remove override of TopLevelOuterWindowId() from NullHttpTransaction and let is use the default one from nsAHttpTransaction.  I want to see if the assert(false) in the default implementation will trigger or not.  I presume it won't.  If yes, it's important to find out where from that null transaction has been queued.

> > @@ +4226,5 @@
> > > +{
> > > +    // Put the transactions from the focused window in the front of the quque.
> > > +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
> > > +    if (!mPendingTransactionTable.Get(currentTopLevelWindowId, &transactionArray)) {
> > > +        return;
> > 
> > should we result.Clear() before return?
> > 
> 
> I'd rather not do it. Since |GetPendingQForFocusedWindow| only append
> transactions in result and not clear the original elements in result, it
> would be better not to clear result for consistency. 

OK.  What I wanted to prevent was this:

{
  nsTArray<> transactions;
  GetPendingQForFocusedWindow(transactions);

  do something with transactions but not clear it

  GetPendingQForFocusedWindow(transactions);
  // when this second call ends up with that early return, 
  // the array will contain the set of transactions from the first call
  // which is probably unexpected - the caller may expect the array be
  // empty when nothing to process was found
}

I hope my explanation is clear enough.

So, please rename GetPendingQForFocusedWindow to AppendPendingQForFocusedWindow, so that it's clear the array will be just appended with found transactions and it's up to a caller to prepare the array as fit.

Comment 23

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> (In reply to Kershaw Chang [:kershaw] from comment #21)
> > > ::: netwerk/protocol/http/NullHttpTransaction.h
> > > @@ +48,5 @@
> > > >    {
> > > >      return PR_SecondsToInterval(15);
> > > >    }
> > > >  
> > > > +  uint64_t TopLevelOuterWindowId() override { return 0; }
> > > 
> > > I think nulltransaction is never put to any queue, so this should be
> > > alright.  what happens when you leave the default impl that asserts?
> > > 
> > 
> > Not sure what you mean here. Do you mean if there is another child class
> > needed to also override |TopLevelOuterWindowId|?
> 
> No, I mean to remove override of TopLevelOuterWindowId() from
> NullHttpTransaction and let is use the default one from nsAHttpTransaction. 
> I want to see if the assert(false) in the default implementation will
> trigger or not.  I presume it won't.  If yes, it's important to find out
> where from that null transaction has been queued.
> 

Null transaction is not in the queue. Where NullHttpTransaction::TopLevelOuterWindowId will be called is at nsHalfOpenSocket::OnOutputStreamReady. The code in my patch is:

> @@ -3484,33 +3600,32 @@ nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream *out)
>      }
>  
>      // This half-open socket has created a connection.  This flag excludes it
>      // from counter of actual connections used for checking limits.
>      mHasConnected = true;
>  
>      // if this is still in the pending list, remove it and dispatch it
>      uint32_t caps = mTransaction->Caps();
> -    int32_t index;
> +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
>      if (caps & NS_HTTP_URGENT_START) {
> -        index = mEnt->mUrgentStartQ.IndexOf(mTransaction);
> +        transactionArray = &mEnt->mUrgentStartQ;
>      } else {
> -        index = mEnt->mPendingQ.IndexOf(mTransaction);
> +        transactionArray =
> +            mEnt->mPendingTransactionTable.Get(mTransaction->TopLevelOuterWindowId());

Since |mTransaction| in nsHalfOpenSocket could be NullHttpTransaction, we still need to override TopLevelOuterWindowId.

> > > @@ +4226,5 @@
> > > > +{
> > > > +    // Put the transactions from the focused window in the front of the quque.
> > > > +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
> > > > +    if (!mPendingTransactionTable.Get(currentTopLevelWindowId, &transactionArray)) {
> > > > +        return;
> > > 
> > > should we result.Clear() before return?
> > > 
> > 
> > I'd rather not do it. Since |GetPendingQForFocusedWindow| only append
> > transactions in result and not clear the original elements in result, it
> > would be better not to clear result for consistency. 
> 
> OK.  What I wanted to prevent was this:
> 
> {
>   nsTArray<> transactions;
>   GetPendingQForFocusedWindow(transactions);
> 
>   do something with transactions but not clear it
> 
>   GetPendingQForFocusedWindow(transactions);
>   // when this second call ends up with that early return, 
>   // the array will contain the set of transactions from the first call
>   // which is probably unexpected - the caller may expect the array be
>   // empty when nothing to process was found
> }
> 
> I hope my explanation is clear enough.
> 
> So, please rename GetPendingQForFocusedWindow to
> AppendPendingQForFocusedWindow, so that it's clear the array will be just
> appended with found transactions and it's up to a caller to prepare the
> array as fit.

Thanks. I get your point now.
I'll fix this in next version.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #23)
> Null transaction is not in the queue. Where
> NullHttpTransaction::TopLevelOuterWindowId will be called is at
> nsHalfOpenSocket::OnOutputStreamReady. The code in my patch is:
> 
> > @@ -3484,33 +3600,32 @@ nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream *out)
> >      }
> >  
> >      // This half-open socket has created a connection.  This flag excludes it
> >      // from counter of actual connections used for checking limits.
> >      mHasConnected = true;
> >  
> >      // if this is still in the pending list, remove it and dispatch it
> >      uint32_t caps = mTransaction->Caps();
> > -    int32_t index;
> > +    nsTArray<RefPtr<nsHttpTransaction>> *transactionArray = nullptr;
> >      if (caps & NS_HTTP_URGENT_START) {
> > -        index = mEnt->mUrgentStartQ.IndexOf(mTransaction);
> > +        transactionArray = &mEnt->mUrgentStartQ;
> >      } else {
> > -        index = mEnt->mPendingQ.IndexOf(mTransaction);
> > +        transactionArray =
> > +            mEnt->mPendingTransactionTable.Get(mTransaction->TopLevelOuterWindowId());

Ah, yes!  The main use of a null trans is at [1].  It is also immediately activated on the connection, so no prioritization is needed.  (Note that the purpose of a null trans is to drive the ssl handshake.)

So, I believe what you've wrote is correct.  Thanks.


[1] https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/netwerk/protocol/http/nsHttpConnectionMgr.cpp#3412

Comment 25

[Kershaw Chang [:kershaw]]

Attachment: patch - v3

Summary:
 - Rebased on the latest code
 - Address the last comments

Please take a look. Thanks.

Comment 26

[Kershaw Chang [:kershaw]]

Attachment: interdiff for v3 and v2

For reference.

Comment 27

[Kershaw Chang [:kershaw]]

Comment on attachment 8847012 [details] [diff] [review]
patch - v3

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +993,5 @@
> +                                                  ent,
> +                                                  considerAll);
> +    }
> +
> +    uint32_t availableCount = AvailableNewConnectionCount(ent);

The availableCount shoud be AvailableNewConnectionCount(ent) + idle urgent connections (connections created to serve urgent request).

We could have more than 6 connections because we create some extra connections for urgent requests. For example, we could have 6 non-urgent active connections and 2 urgent connections. When those transactions are done, we have 8 idle connections now. In this case, AvailableNewConnectionCount only returns 6. To utilize all 8 idle connections, we should also consider the extra connections created for urgent requests.
However, we didn't track the number of extra connections in bug 1312774. To put it simply, maybe we can change to: availableCount = AvailableNewConnectionCount(ent) + mMaxUrgentStartQ? After all, getting more transactions for dispatching should be harmless?

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #27)
> Comment on attachment 8847012 [details] [diff] [review]
> patch - v3
> 
> Review of attachment 8847012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +993,5 @@
> > +                                                  ent,
> > +                                                  considerAll);
> > +    }
> > +
> > +    uint32_t availableCount = AvailableNewConnectionCount(ent);
> 
> The availableCount shoud be AvailableNewConnectionCount(ent) + idle urgent
> connections (connections created to serve urgent request).
> 
> We could have more than 6 connections because we create some extra
> connections for urgent requests. For example, we could have 6 non-urgent
> active connections and 2 urgent connections. When those transactions are
> done, we have 8 idle connections now. In this case,
> AvailableNewConnectionCount only returns 6. To utilize all 8 idle
> connections, we should also consider the extra connections created for
> urgent requests.
> However, we didn't track the number of extra connections in bug 1312774. To
> put it simply, maybe we can change to: availableCount =
> AvailableNewConnectionCount(ent) + mMaxUrgentStartQ? After all, getting more
> transactions for dispatching should be harmless?

For now I would not bother with urgent "leftovers".  I have some plans to go back to 6 connections after some time, leaving the least utilized connection just automatically close.  If your algorithm, as is, supports that, just leave it for now.

Comment 29

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Kershaw Chang [:kershaw] from comment #27)
> > Comment on attachment 8847012 [details] [diff] [review]
> > patch - v3
> > 
> > Review of attachment 8847012 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> > @@ +993,5 @@
> > > +                                                  ent,
> > > +                                                  considerAll);
> > > +    }
> > > +
> > > +    uint32_t availableCount = AvailableNewConnectionCount(ent);
> > 
> > The availableCount shoud be AvailableNewConnectionCount(ent) + idle urgent
> > connections (connections created to serve urgent request).
> > 
> > We could have more than 6 connections because we create some extra
> > connections for urgent requests. For example, we could have 6 non-urgent
> > active connections and 2 urgent connections. When those transactions are
> > done, we have 8 idle connections now. In this case,
> > AvailableNewConnectionCount only returns 6. To utilize all 8 idle
> > connections, we should also consider the extra connections created for
> > urgent requests.
> > However, we didn't track the number of extra connections in bug 1312774. To
> > put it simply, maybe we can change to: availableCount =
> > AvailableNewConnectionCount(ent) + mMaxUrgentStartQ? After all, getting more
> > transactions for dispatching should be harmless?
> 
> For now I would not bother with urgent "leftovers".  I have some plans to go
> back to 6 connections after some time, leaving the least utilized connection
> just automatically close.  If your algorithm, as is, supports that, just
> leave it for now.

If so, we have to modify test_bug1312774_http1.js, otherwise we could have an intermittent failure like [1].
In test_bug1312774_http1.js, we assume that http server should receive all 6 common http requests before 2 urgent http requests. However, it is possible that 2 urgent requests are dispatched before other common requests. In this case, 4 of 6 active connections are used by common http requests and the rest 2 are used by urgent requests. At this point, the rest 2 common http requests have no chance to be dispatched because all 6 connections are all being taken.

I think we should create 2 urgent http requests until server really receive all 6 common requests. I'll upload a patch later and ask for your review.


[1] https://treeherder.mozilla.org/logviewer.html#?job_id=83945218&repo=try&lineNumber=5417

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #29)
> If so, we have to modify test_bug1312774_http1.js, otherwise we could have
> an intermittent failure like [1].

Yes!  Good catch.

> I think we should create 2 urgent http requests until server really receive
> all 6 common requests. I'll upload a patch later and ask for your review.

Agree.  It's sufficient to keep the test testing what it should and also be less racy.  Thanks.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8847012 [details] [diff] [review]
patch - v3

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

Please be aware of bugs: 1347951 1348081 1348016.  You will need to merge and then make sure you don't introduce them again.  I would like to r that again then, otherwise this would be r+.

To sum, this is a good patch for basic per-origin prioritization.  I'll soon bring a plan on how to handle cross-origin prioritization also in relation to bug 1348035, so there will be more work.

::: netwerk/protocol/http/NullHttpTransaction.h
@@ +48,5 @@
>    {
>      return PR_SecondsToInterval(15);
>    }
>  
> +  uint64_t TopLevelOuterContentWindowId() override { return 0; }

please write a good comment why this is OK - it's important (I think I commented about this bit in this bug somewhere..)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +967,5 @@
> +         "total connection count = %d, limit %d\n",
> +         totalCount, maxPersistConns));
> +
> +    return maxPersistConns > totalCount ?
> +           maxPersistConns - totalCount :

nit: only my personal preference of formatting for ?:

return maxPersistConns > totalCount
  ? maxPersistConns > totalCount
  : 0;

@@ +994,5 @@
> +                                                  considerAll);
> +    }
> +
> +    uint32_t availableCount = AvailableNewConnectionCount(ent);
> +    // No need to try diapcthing if we reach the active connection limit.

diapcthing

@@ +1024,5 @@
> +            nonFocusedWindowTrans,
> +            maxNonFocusedWindowConnections);
> +    }
> +
> +    // Try to fill both queues as many as possible.

a better comment would be:

// if the slots for either non-focused or focused are not filled up to the availability, try to use the remaining available connections for the other slot (with preference for the focused window).

@@ +1052,5 @@
> +    // Put the leftovers into connection entry
> +    for (const auto& transaction : focusedWindowTrans) {
> +        ent->InsertTransaction(transaction);
> +    }
> +    focusedWindowTrans.Clear();

I honestly don't think you need to explicitly clear this local array.

@@ +1750,5 @@
>              InsertTransactionSorted(ent->mUrgentStartQ, trans);
>          } else {
>              LOG(("  adding transaction to pending queue "
>                   "[trans=%p pending-count=%" PRIuSIZE "]\n",
> +                 trans, ent->PendingQLength()+1));

when you are here, please spaces around +

@@ +1899,5 @@
> +    // Put the leftovers back in the pending queue.
> +    for (const auto& trans : pendingQ) {
> +        ent->InsertTransaction(trans);
> +    }
> +    pendingQ.Clear();

no need to call Clear()

@@ +2353,5 @@
> +            for (auto it = ent->mPendingTransactionTable.Iter();
> +                 !it.Done();
> +                 it.Next()) {
> +                it.UserData()->Compact();
> +            }

we should also delete arrays with no elements from the hash table.  it's often iterated and keeping old arrays here would slow eventually down.  should be done before the big condition above.

@@ +3639,5 @@
>  }
>  
> +size_t
> +nsHttpConnectionMgr::
> +nsConnectionEntry::PendingQLength() const

keep on one line please

@@ +3643,5 @@
> +nsConnectionEntry::PendingQLength() const
> +{
> +  size_t length = 0;
> +  for (auto it = mPendingTransactionTable.ConstIter(); !it.Done(); it.Next()) {
> +    length += it.UserData()->Length();

maybe delete empty arrays here?

@@ +3696,5 @@
> +}
> +
> +void
> +nsHttpConnectionMgr::
> +nsConnectionEntry::GetRestPendingTransactions(

GetRemaning... or GetPendingQForUnfocusedWindows

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +215,5 @@
>  
>          RefPtr<nsHttpConnectionInfo> mConnInfo;
>          nsTArray<RefPtr<nsHttpTransaction> > mUrgentStartQ;// the urgent start transaction queue
> +        nsClassHashtable<nsUint64HashKey,
> +                         nsTArray<RefPtr<nsHttpTransaction>>> mPendingTransactionTable;

add a comment what is the key, also describe if and how is 0 id used.

@@ +264,5 @@
>          void RecordIPFamilyPreference(uint16_t family);
>          // Resets all flags to their default values
>          void ResetIPFamilyPreference();
> +
> +        // Return the count of pending transactions

for all window ids

@@ +267,5 @@
> +
> +        // Return the count of pending transactions
> +        size_t PendingQLength() const;
> +
> +        // Add a transaction into mPendingTransactionTable.

how?  please add more details

@@ +270,5 @@
> +
> +        // Add a transaction into mPendingTransactionTable.
> +        void InsertTransaction(nsHttpTransaction *trans);
> +
> +        // Append transactions whose window id is equal to currentTopLevelWindowId.

Append transactions to the result arg whose ...

@@ +275,5 @@
> +        // NOTE: maxCount == 0 will get all transactions.
> +        void AppendPendingQForFocusedWindow(
> +            uint64_t currentTopLevelWindowId,
> +            nsTArray<RefPtr<nsHttpTransaction>> &result,
> +            uint32_t maxCount);

maybe make default maxCount = 0?

@@ +279,5 @@
> +            uint32_t maxCount);
> +
> +        // Get transactions whose window id isn't equal to currentTopLevelWindowId.
> +        // NOTE: currentTopLevelWindowId == 0 will get all transactions for both
> +        // focused and non-focused window.

focused and non-focused windowS.

@@ +409,4 @@
>                                     bool considerAll);
> +
> +    // Return the available count for new connections.
> +    uint32_t AvailableNewConnectionCount(nsConnectionEntry * ent);

the comment is too vague.  can you rephrase or add more details?

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1457,5 @@
> +            if (ratio > 0 && ratio < 1) {
> +                mFocusedWindowTransactionRatio = ratio;
> +            } else {
> +                MOZ_RELEASE_ASSERT(
> +                    false, "Wrong value for focused_window_transaction_ratio");

please don't.  just warn and ignore with reverting to the default

Comment 32

[Honza Bambas (:mayhemer)]

Kershaw, I'll ask you for one more change, it just a few lines of code removal at nsHttpConnectionMgr::ProcessPendingQForEntry.  It's the "// Try to fill both queues as many as possible." piece.  I want you to remove the part that fills up the non-focused window slot - the |else if (focusedWindowTrans.Length() < maxFocusedWindowConnections)| branch.  

The result would be that a background tab won't get more than a |the ratio * 6| connections per a host.

The reason is to give as much as possible bandwidth to the active tab.  If there happen to be background tabs with number of origins different from the set of origins the active tab requests from, we would use most of the bandwidth for background tabs and not the active tab.

Thanks.

Comment 33

[Honza Bambas (:mayhemer)]

Comment 32

Comment 34

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #33)
> Comment 32

Sure. I'll do it.

Comment 35

[Kershaw Chang [:kershaw]]

Attachment: patch - v4

Summary:
 - Rebased
 - Address previous comments
 - In |ProcessPendingQForEntry|, only dispatch transactions for either focused or non-focused window when considerAll is false (inspired by bug 1348081).
 - Removing empty pending queues in |PendingQLength| seems strange to me, instead, I think we should do it in |OnMsgPruneDeadConnections| and at the end of |ProcessPendingQForEntry| (when considerAll is true).

Comment 36

[Kershaw Chang [:kershaw]]

Attachment: modify test case

Modify test_bug1312774_http1.js as described in comment #29.

Comment 37

[Kershaw Chang [:kershaw]]

Attachment: test case

Summary:
 - Inspired from bug 1312774, create a test to check if we send the http requests from the focused window first.
 - Add topLevelOuterContentWindowId attribute in nsIHttpChannel.idl. This attribute is only for test purpose. Not sure if we really want this.

Please take a look. Thanks!

Comment 38

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2294b819b931233774368f7c2a98b4fd231b81aa

Comment 39

[Honza Bambas (:mayhemer)]

Comment on attachment 8849924 [details] [diff] [review]
patch - v4

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

not locally tested.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1038,5 @@
>      if (dispatchedSuccessfully && !considerAll) {
>          return dispatchedSuccessfully;
>      }
> +
> +    uint32_t availableCount = AvailableNewConnectionCount(ent);

s/availableCount/availableConnections/

@@ +1098,5 @@
> +    }
> +
> +    // If the slots for the non-focused window are not filled up to
> +    // the availability, try to use the remaining available connections
> +    // for the other slot (with preference for the focused window).

for the focused window.

(it's clearer)

@@ +2362,5 @@
>      return PostEvent(&nsHttpConnectionMgr::OnMsgCancelTransactions, intReason, ci);
>  }
>  
>  void
> +nsHttpConnectionMgr::CancelTransactionsHelper(

could this be just static?

@@ +3835,5 @@
> +{
> +  LOG(("nsHttpConnectionMgr::nsConnectionEntry::InsertTransaction"
> +       " trans=%p, windowId=%" PRIu64 "\n",
> +       info->mTransaction.get(),
> +       info->mTransaction->TopLevelOuterContentWindowId()));

blank line after the log please

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +289,5 @@
> +        // NOTE: maxCount == 0 will get all transactions in the queue.
> +        void AppendPendingQForFocusedWindow(
> +            uint64_t windowId,
> +            nsTArray<RefPtr<PendingTransactionInfo>> &result,
> +            uint32_t maxCount);

maxCount = 0 (defaul) here as well?

@@ +294,5 @@
> +
> +        // Get transactions whose window id isn't equal to |windowId|.
> +        // NOTE: windowId == 0 will get all transactions for both
> +        // focused and non-focused windows.
> +        void GetRemainingPendingTransactions(

Sorry to bring a name change here again..  but this should be AppendPendingQForNonFocusedWindows().

@@ +360,5 @@
>  
>          void PrintDiagnostics(nsCString &log);
>      private:
> +        already_AddRefed<PendingTransactionInfo>
> +        FindTransactionHelper(bool removeWhenFound);

comment what this is.

@@ +527,5 @@
> +    void CancelTransactionsHelper(
> +        nsTArray<RefPtr<PendingTransactionInfo>> &pendingQ,
> +        const nsHttpConnectionInfo *ci,
> +        const nsConnectionEntry *ent,
> +        nsresult reason);

again, comment what this is doing

Comment 40

[Honza Bambas (:mayhemer)]

Comment on attachment 8849928 [details] [diff] [review]
modify test case

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

thanks!

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 8850476 [details] [diff] [review]
test case

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

Looks good, but didn't have strength to test this locally.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +471,5 @@
>      [infallible] readonly attribute boolean isTrackingResource;
> +
> +    /**
> +     * ID of the top-level outer content window. Identifies this channel's
> +     * focused window.

not focused window.  the window it comes from.

please add a note this is exposed mainly for purpose of simpler automated testing (xpcshell test) and should not be altered otherwise.  I can't say I'm super happy with having a setter like this, but maybe it will find its use not just in tests (Bug 1350374)

::: netwerk/test/unit/test_bug1312782_http1.js
@@ +17,5 @@
> +//    until get all 6 requests.
> +// 4. When the server receive all 6 requests, starts to check that the request ids of
> +//    the first 4 requests in the queue should be all less than focused window id
> +//    plus 4. Also, the request ids of the rest requests should be less than non-focused
> +//    window id + 2.

thanks!

@@ +160,5 @@
> +  });
> +
> +}
> +
> +function processResponse() {

should be processResponses()

Comment 42

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #39)
> Comment on attachment 8849924 [details] [diff] [review]
> patch - v4
> 
> Review of attachment 8849924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> not locally tested.
> 

Thanks for the review.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +1038,5 @@
> >      if (dispatchedSuccessfully && !considerAll) {
> >          return dispatchedSuccessfully;
> >      }
> > +
> > +    uint32_t availableCount = AvailableNewConnectionCount(ent);
> 
> s/availableCount/availableConnections/
>
 
Done.

> @@ +1098,5 @@
> > +    }
> > +
> > +    // If the slots for the non-focused window are not filled up to
> > +    // the availability, try to use the remaining available connections
> > +    // for the other slot (with preference for the focused window).
> 
> for the focused window.
> 
> (it's clearer)
> 

Done.

> @@ +2362,5 @@
> >      return PostEvent(&nsHttpConnectionMgr::OnMsgCancelTransactions, intReason, ci);
> >  }
> >  
> >  void
> > +nsHttpConnectionMgr::CancelTransactionsHelper(
> 
> could this be just static?
> 

Unfortunately not because PendingTransactionInfo is a private member in nsHttpConnectionMgr (the same reason that InsertTransactionSorted also becomes a private function in nsHttpConnectionMgr).


> @@ +3835,5 @@
> > +{
> > +  LOG(("nsHttpConnectionMgr::nsConnectionEntry::InsertTransaction"
> > +       " trans=%p, windowId=%" PRIu64 "\n",
> > +       info->mTransaction.get(),
> > +       info->mTransaction->TopLevelOuterContentWindowId()));
> 
> blank line after the log please
> 

Done.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +289,5 @@
> > +        // NOTE: maxCount == 0 will get all transactions in the queue.
> > +        void AppendPendingQForFocusedWindow(
> > +            uint64_t windowId,
> > +            nsTArray<RefPtr<PendingTransactionInfo>> &result,
> > +            uint32_t maxCount);
> 
> maxCount = 0 (defaul) here as well?
> 

Done.

> @@ +294,5 @@
> > +
> > +        // Get transactions whose window id isn't equal to |windowId|.
> > +        // NOTE: windowId == 0 will get all transactions for both
> > +        // focused and non-focused windows.
> > +        void GetRemainingPendingTransactions(
> 
> Sorry to bring a name change here again..  but this should be
> AppendPendingQForNonFocusedWindows().
> 

Done.

> @@ +360,5 @@
> >  
> >          void PrintDiagnostics(nsCString &log);
> >      private:
> > +        already_AddRefed<PendingTransactionInfo>
> > +        FindTransactionHelper(bool removeWhenFound);
> 
> comment what this is.
> 

Done.

> @@ +527,5 @@
> > +    void CancelTransactionsHelper(
> > +        nsTArray<RefPtr<PendingTransactionInfo>> &pendingQ,
> > +        const nsHttpConnectionInfo *ci,
> > +        const nsConnectionEntry *ent,
> > +        nsresult reason);
> 
> again, comment what this is doing

Done.

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Part1: Implement window id to pending transactions table, r=honzab

Summary:
 - Address the previous comments.
 - Carry reviewer's name.

Comment 44

[Kershaw Chang [:kershaw]]

Attachment: Part2: Create urgent http requests until server receive all common http requests, r=honzab

Carry reviewer's name.

Comment 45

[Kershaw Chang [:kershaw]]

Attachment: Part3: Test case, r=honzab

Carry reviewer's name.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90b77144005
Part 1: Implement window id to pending transactions table. r=honzab
https://hg.mozilla.org/integration/mozilla-inbound/rev/a273bebb4eec
Part 2: Create urgent http requests until server receive all common http requests. r=honzab
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2c6d0f6db6
Part 3: Test case. r=honzab

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e90b77144005
https://hg.mozilla.org/mozilla-central/rev/a273bebb4eec
https://hg.mozilla.org/mozilla-central/rev/eb2c6d0f6db6