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

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 2 obsolete attachments)

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
Status: NEW → ASSIGNED
Priority: -- → P2
Posted patch wip1 (obsolete) — Splinter Review
Whiteboard: [necko-triaged]
Posted 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]
wip2

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)
Posted patch v1Splinter Review
Locally tested, observed very good performance when loading multiple pages from the same domain (e.g bbc.com).  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+
thanks!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6bd79484f23
Connections created for urgent-start requests are of limits for non-urgent-start ones, r=dragana
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6bd79484f23
Status: ASSIGNED → RESOLVED
Closed: 2 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.