Closed Bug 1369496 Opened 3 years ago Closed 3 years ago

Improve background http transaction throttling algorithm


(Core :: Networking: HTTP, defect)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: mayhemer, Assigned: mayhemer)




(1 file, 2 obsolete files)

Testing shows that the form as the patch for bug 1365307 lands doesn't bring much improvement or may even be worse (hard to assess tho, jitter is too high).

Suggested improvements (possibly subject to change):
- decrease the resume-for time to 100ms
- increase the resume-background-in delay to 1000ms
- delay-resume all background transactions, not just throttled
- don't throttle (stop reading) when at least 1.5k has been read to let go extra small responses that doesn't block anyway and are already read in system buffers
- introduce "entry pressure" for h1: when there are transactions for the active tab id pending on the entry (queued later than the throttled transaction has been activated), then don't throttle a transaction even though that transaction conforms all conditions to stop reading ; the purpose here is to free up the connection for active tab transactions (apparently all are taken by background tabs), presuming that the scheduler is smart enough to pick them over any background tab transactions
Assignee: nobody → honzab.moz
Attached patch wip1 (obsolete) — Splinter Review
with this patch I'm getting slight improvements when loading 8 background tabs (no cache) + clicking a direct link on in the active tab (homepage).

- pref changes (probably lower the suspend-for time a bit - ~900ms)
- or when pressure on an entry is made, unthrottle active transactions for the entry (expensive)
Attached patch v1 (obsolete) — Splinter Review
result of few hours of manual testing, some measurements and overall feeling being better with this patch.

- a transaction does not stop reading when its connection entry has pending transactions for the active tab ; this makes the connection being freed up sooner
- a transaction does not stop reading until at least around 1.5k of content has been read ; there are often many small responses (including redirects) that are illogical to throttle
- suspend-for changed to 900 ; when background transactions occupy the whole 6 conn limit and the active tab creates a new request, there is a smaller delay to make the background transactions to wake up fully to free up a connection (yes, a wake-up-on-queue solution would be better here, but let's do it in yet a different bug)
- to minimize background transaction influence during the active tab's page load phase the background resume postpone delay has been increased to 1s
- the delayed background resume timer is not canceled automatically when the resume reading tick fires ; this is obvious when we want to make the resume-background-in interval longer or near the same as the resume-for interval
- background transactions are always throttled when the delayed resume timer is scheduled (while the ticker is at the 'stop reading' state, of course)
Attachment #8873614 - Attachment is obsolete: true
Attachment #8874440 - Flags: review?(mcmanus)
Comment on attachment 8874440 [details] [diff] [review]

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +834,5 @@
> +        // We are not obligated to throttle
> +        return false;
> +    }
> +
> +    if (mContentRead < 1450) {

honestly, I think you can easily make this 10x bigger (i.e. 16KB).. the order of magnitude of flow control etc is so much bigger than your constant that you won't really have much of an impact on bottleneck contention, but you will delay delivery to the application of stuff sitting in socket buffers.

you should at least experiment with it as a followon adjustment
Attachment #8874440 - Flags: review?(mcmanus) → review+
Attached patch v1.1Splinter Review
- 16k limit to start throttling
Attachment #8874440 - Attachment is obsolete: true
Attachment #8875789 - Flags: review+
Keywords: checkin-needed
Pushed by
HTTP transaction read throttling algorithm improvements. r=mcmanus
Keywords: checkin-needed
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1371649
Depends on: 1373064
You need to log in before you can comment on or make changes to this bug.