Closed Bug 891932 Opened 6 years ago Closed 6 years ago

nsHttpConnectionMgr::ProcessSpdyPendingQ reverses queue order

Categories

(Core :: Networking: HTTP, defect, minor)

x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mcmanus, Assigned: u408661)

Details

(Whiteboard: [spdy])

Attachments

(1 file)

This is a pretty minor bug.

ProcessSpdyPendingQ() takes transactions queued at the connection manager level and submits them to an active spdy session.. the only problem is that it submits them from mPendingQ position N down to 0, and that queue is ordered by priority. It does it because it is deleting elements from the queue as it goes so that simplifies the code a bit.

In the spdy context this is pretty minor because:

1] unlike HTTP/1, its rare to get a queue of things at all in the connection manager - normally we can dispatch immediately due to spdy mux. The exception to this is when we are handshaking a connection that we don't yet know is spdy - during that time the queue can build.

2] Even if a queue has built up, it is probably all splat out to the wire in one iteration of this list.. so nobody is left behind, again thanks to spdy mux. And the spdy requests will carry a priority with them, so to the extent that different items in the queue have different explicit priorities (not just implicit ordered priorities) it will all be pretty good.

But I think its worth fixing anyhow.. We have lots of resources with essentially tied priorities and a server-side RR algorithm will serve them (at least partially) FIFO.. so reversing that list isn't awesome... and while spdy does have good mux, the server can advertise limits to that - as low as 10 in some cases, so queues can still come into play.

nick, interested in fixing?
Yeah, I'll put this one on my plate
Assignee: nobody → hurley
Attached patch patchSplinter Review
Here's a patch. Seems to work fine browsing around gmail, google cal, docs, etc, and passes tests on try.
Attachment #780591 - Flags: review?(mcmanus)
Comment on attachment 780591 [details] [diff] [review]
patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1959,5 @@
>  void
>  nsHttpConnectionMgr::ProcessSpdyPendingQ(nsConnectionEntry *ent)
>  {
>      nsHttpConnection *conn = GetSpdyPreferredConn(ent);
>      if (!conn)

as an optimization, change that to 

jf (!conn || !conn->CanDirectlyActivate())
 return;
Attachment #780591 - Flags: review?(mcmanus) → review+
> as an optimization, change that to 
> 
> jf (!conn || !conn->CanDirectlyActivate())
>  return;

Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ade076935d18
https://hg.mozilla.org/mozilla-central/rev/ade076935d18
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.