Returning leftover http transactions after unsuccessful dispatch ruins natural ordering

RESOLVED FIXED in Firefox 57

Status

()

Core
Networking: HTTP
P1
major
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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.
(Assignee)

Updated

2 months ago
Priority: -- → P1
(Assignee)

Comment 1

2 months ago
Created attachment 8899469 [details] [diff] [review]
v1
Attachment #8899469 - Flags: review?(kechang)
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?
(Assignee)

Comment 3

2 months ago
(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.
> 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.
Attachment #8899469 - Flags: review?(kechang) → review+
(Assignee)

Comment 5

2 months ago
Thanks!
Keywords: checkin-needed
(Assignee)

Updated

2 months ago
Whiteboard: [necko-active]

Comment 6

2 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 7

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c092fcf5ef9a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.