1362071 - [meta] Improve h1 connection/h2 stream download throttling

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

Details to come.

Comment 1

[Honza Bambas (:mayhemer)]

Implementation details:

- throttle solely by stopping read from the socket: this will be implemented in nsHttpTransaction, simply when a transaction is about to stop reading, it returns NS_ERROR_WOULD_BLOCK from nsHttpTransaction::WriteSegments.  this makes either nsHttpConnection or Http2Stream wait for call to ResumeRecv on itself ; hence no need for bug 1355782, this will protocol-independent 

- remove the code that suspends and resumes channels on pressure periodically since it doesn't make much sense to keep it
- we also don't need to keep the pressure indication at all, enough is to track number of active transactions per-window

- keep active transactions (on the wire) in two hashtables by window id, one for non-throttled (mUnthrottledTransx) and one for throttled transactions (mThrottledTransx) ; "throttled" means having the Throttleable class flag ; must keep in mind the flag can be set at any time during the transaction lifetime, hence active transactions must "travel" between the two hashtables (all on socket thread only)

- an active transaction T should be throttled when tab A is active as (ShouldThrottle(T), checked in each call of nsHttpTransaction::WriteSegments, hence, must be optimized):

  if (mUnthrottledTransx[A].count() || mThrottledTransx[A].count()) {
    // Something happens for the active tab
    if (!T.ForTab(A)) 
      return true; // this is a background tab load
    if (mUnthrottledTransx[A].count() > 0) 
      return T.class & Throttleable; // there are unthrottled tranx in progress, respect our throttling
    return false;
  }
  // the active tab is not loading anything

  if (mUnthrottledTransx[*].count() > 0)
    return T.class & Throttleable; // there are in progress unthrottleable tranx, respect own throttling
  return false; // there are only throttleable transactions left in progress


- there should be a single timer (as we have now), that periodically ticks
  - the ticker (3s/200ms) will set/unset a global "stop reading" flag
  - on unset of the flag by the timer
    -> iterate all m(Un)ThrottledTransx arrays to wake them up (except the mUntrottledTransx[A], obviously)

- on add to any of m(Un)TrottledTransx and timer being off
  -> set the "stop reading" global flag and start the timer

- when "stop reading" is ON:
  - on change to the active tab (A is the new active tab id)
    -> iterate all mUnthrottledTransx[A] arrays to wake them up, if non-empty, and exit
    -> iterate all mThrottledTransx[A] arrays to wake them up, if non-empty, and exit
    -> iterate all remaning mUnthrottledTransx arrays to wake them up, exit if some woken
    -> iterate all remaning mThrottledTransx arrays to wake them up, exit if some woken
  - on emptying of mUnthrottledTransx[A]
    -> iterate all mThrottledTransx[A] arrays to wake them up, if non-empty, and exit
    -> iterate all remaning mUnthrottledTransx arrays to wake them up, exit if some woken
    -> iterate all remaning mThrottledTransx arrays to wake them up, exit if some woken
  - on emptying of mThrottledTransx[A] while also mUnthrottledTransx[A] being empty
    -> iterate all remaning mUnthrottledTransx arrays to wake them up, exit if some woken
    -> iterate all remaning mThrottledTransx arrays to wake them up, exit if some woken
  - similarly for complete of non-active...
  - and finally, when nothing active 
    -> stop the timer and make sure "stop reading" is false

- note that there is no need to 'put transx to sleep' since all will automatically stop reading based on result of ShouldThrottle that will automatically drop to false when conditions change


This should, if present, throttle everything but:
1. non-throttled for the active tab
2. throttled for the active tab
3. non-throttled for background tabs
4. throttled for background tabs

Nice is that we can play with the "0"s in ShouldThrottle function.  We could allow some threshold when to open floodgates.

I will split this to separate bugs.

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 3

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 4

[Honza Bambas (:mayhemer)]

I don't think we need to track the remaining issue.