Closed Bug 1405446 Opened 3 years ago Closed 3 years ago

h1 urgent start connections should be used only for urgent start requests


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




Tracking Status
firefox58 --- wontfix
firefox59 --- fixed


(Reporter: mayhemer, Assigned: mayhemer)


(Blocks 1 open bug)


(Whiteboard: [necko-triaged])


(1 file, 2 obsolete files)

since otherwise we may soon exhaust/block them when multiple pages to the same hosts are open or when a single user interaction leads to more then 4 requests.

this means marking a connection as urgent-start (flag) and probably also effectively renders bug 1345845 invalid, since urgent start conns will, when not utilized, close soon by our timeout.
Assignee: nobody → honzab.moz
Priority: -- → P2
Attached patch wip1 (obsolete) — Splinter Review
Attached patch wip2 (obsolete) — Splinter Review
still has issues.  I can clearly see the improvement when loading multiple pages from the same domain - we immediately load the html in every case - but there is another issue: parallelism sometimes go to hell for the active page.  I'm not sure right now about the cause.

I have to investigate a bit more time to this, but according importance, I don't think I want right now.
Attachment #8917474 - Attachment is obsolete: true
Comment on attachment 8918915 [details] [diff] [review]

Review of attachment 8918915 [details] [diff] [review]:

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1609,5 @@
> +        // to dispatch on an urgent-start-only marked connection to avoid
> +        // dispatch deadlocks
> +        if (!(trans->ClassOfService() & nsIClassOfService::UrgentStart) &&
> +            idleConnsAllUrgent &&
> +            !ent->mActiveConns.Length()) {

ent->mActiveConns.Length() < 6 will probably do.  I think this is the point ruining parallelism.  When there are only urgent idle conns, we use one of them here (becomes active).  for another trans in the row this cond will no pass (!ent->mActiveConns.Length() evals to false)
Attached patch v1Splinter Review
Locally tested, observed very good performance when loading multiple pages from the same domain (e.g  Parallelism doesn't suffer.

how urgent dispatching works w/o this patch:
- we have a regular limit of 6 conns per host
- we can create up to 4 more conns for urgent transactions (total of 10 conns)
- all these are then used for both regular and urgent transactions to dispatch, allowing to make the limit higher and when all used, disallow usage of the urgent-start feature at all..

the goal of the patch is to limit number of active connections carrying only regular transactions back to 6 while allowing connections that are urgent-preferred free to reuse by leaving them idle ; these urgent-preferred conns will be used for any urgent transactions immediately

when it happens that there is e.g 8 urgent conns and 2 regular, we allow usage of urgent-preferred conns for regular transactions when active conns count (both regular and urgent summed) is less than 6

this ensures that parallelism is 6 while we save the reserve for urgency when it comes

in the patch:
- marks an nsHttpConnection resulting from urgent-start initiated speculative connection as "urgent preferred"
- marks an nsHttpConnection resulting from a regular speculative connection as "urgent preferred" when the first non-null transaction dispatched on it is urgent
- when trying to dispatch a transaction (from the queue or a new high-prio one) which is regular, we first look for a regular idle conn ; if none is found, but we can't make new connections because of the 6 per-host limit although active connections are less than 6 (so that we can dispatch), we force use of an urgent-preferred conn for a regular transaction (the "repeat step 2" part)
Attachment #8918915 - Attachment is obsolete: true
Attachment #8941096 - Flags: review?(dd.mozilla)
Attachment #8941096 - Flags: review?(dd.mozilla) → review+
Keywords: checkin-needed
Pushed by
Connections created for urgent-start requests are of limits for non-urgent-start ones, r=dragana
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Duplicate of this bug: 1345845
You need to log in before you can comment on or make changes to this bug.