1392264 - Returning leftover http transactions after unsuccessful dispatch ruins natural ordering

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

This is a major perf problem.  We can't prioritize images sooner than during the layout phase which is too late, preload already kicked in most cases.  Hence, leaving trans at least in the natural order is vital to load images from the top to the bottom as they are placed in the markup (for most modern webs it usually means the order of appearance on the screen).

Will have a patch momentarily.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 2

[Kershaw Chang [:kershaw]]

Comment on attachment 8899469 [details] [diff] [review]
v1

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1173,5 @@
>  
> +    // Put the leftovers into connection entry, in the same order as they
> +    // were before to keep the natural ordering.
> +    for (const auto& transactionInfo : Reversed(pendingQ)) {
> +        ent->InsertTransaction(transactionInfo, true);

I think only putting the leftovers back in reverse is not enough.

For example, there is a pendingQ [A0, B0, C0, D0], where A0, B0, C0, and D0 all have priority 0.

If we select A0 and B0 out and put them back in reverse, the pendingQ would be like:
[C0, D0, B0, A0].

I think it's difficult to keep pendingQ's order as before. What do you think?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #2)
> Comment on attachment 8899469 [details] [diff] [review]
> v1
> 
> Review of attachment 8899469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +1173,5 @@
> >  
> > +    // Put the leftovers into connection entry, in the same order as they
> > +    // were before to keep the natural ordering.
> > +    for (const auto& transactionInfo : Reversed(pendingQ)) {
> > +        ent->InsertTransaction(transactionInfo, true);
> 
> I think only putting the leftovers back in reverse is not enough.
> 
> For example, there is a pendingQ [A0, B0, C0, D0], where A0, B0, C0, and D0
> all have priority 0.
> 
> If we select A0 and B0 out and put them back in reverse, the pendingQ would
> be like:
> [C0, D0, B0, A0].
> 
> I think it's difficult to keep pendingQ's order as before. What do you think?

No.  The code as I change it adds the transactions back correctly, note the second arg being true.  It adds each transaction (one by one in the reversed order) as the first before a trans with the same priority, hence you reconstruct the array the same way as it was before.  I locally tested this, it's correct.

Comment 4

[Kershaw Chang [:kershaw]]

> No.  The code as I change it adds the transactions back correctly, note the
> second arg being true.  It adds each transaction (one by one in the reversed
> order) as the first before a trans with the same priority, hence you
> reconstruct the array the same way as it was before.  I locally tested this,
> it's correct.

Yes, you are right. I missed the second argument.

Comment 5

[Honza Bambas (:mayhemer)]

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c092fcf5ef9a
Return undispatched HTTP transaction to the queue in the same order as they were before. r=kershaw

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c092fcf5ef9a