Honza Bambas (:mayhemer) Assignee
	Description • 7 years ago

according bug 1362071 comment 1.

	Honza Bambas (:mayhemer) Assignee
	Comment 3 • 7 years ago

One small implementation details I definitely want in the patch: when the last transaction is removed from a respective array in any of the m(Un)Throttleable hash tables, remove the array (the item for the tab id) completely.  It will then reduce the |mUnthrottledTransx[*].count() > 0| [1] condition to just a simple and very fast check of |mUnthrottledTransx.count() > 0|.  No need to recalculate or cache presence of unthrottled transactions for background tabs.

[1] bug 1362071 comment 1

	Honza Bambas (:mayhemer) Assignee
	Comment 4 • 7 years ago

(backup)

This has one fundamental problem: it may happen that transactions for the active window can quite often during the page loading process drop to zero.  That makes any downloads (or anything that should be throttled) start reading again.  I have to test if this is a problem or not (my guts tell me it's not the most efficient way to throttle, the effect will be ruined with this IMO).

Anyway, this is a good base, I believe.

	Honza Bambas (:mayhemer) Assignee
	Comment 5 • 7 years ago

- "net:current-toplevel-outer-content-windowid" enhanced to provide and notify also whether/when the active is in progress of the page load or not
- this whole code strongly depends on bug 1366822 resolution

If the solution for bug 1366822 can't easily provide the "loading/not-loading" notifications, this could be worked around by introducing another timer limit (~500ms) to resume any background/download throttled transactions after all active tab transactions has finished.  That may well span intervals between e.g. html download finish and start of a css load for the active tab and prevent premature resume of downloads (that would be throttled immediately again anyway).

	Honza Bambas (:mayhemer) Assignee
	Comment 6 • 7 years ago

- reverts the "is the active tab loading a page" notification
- added a 300 ms timer to resume background-throttled transactions *) when nothing else is active ; this nicely solves the issue universally also for background tab loads
- overall, this is close to the final version! (still needs cleanup and prefs)

No idea how to test this well....

*) that means mainly only downloads or TP (when marked) and a like resources from background tabs we may delay resume for 300ms, maybe even more

	Honza Bambas (:mayhemer) Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c29297a12a59c96d82a112aed9cfc1d5b717f06

for implementation details you can check bug 1362071 comment 1

in a nutheshell:
- there are four "classes" of transactions
 1. for active tab and unthrottleable*
 2. for active tab and throttleable 
 3. background tab (or tabid=0) and unthrottleable
 4. background tab (or tabid=0) and throttleable
* (un)throttleable means whether the Throttleable class of service flag has been set.

- represented by two hashtables of tabid->trans[], one for unthr-able, other for thr-able, only activated transactions are kept there until done

- If there is any trans of a "higher" class present then all transactions of all "lower" classes are throttled (regardless of the Throttleable class flag)
- exception: transactions with tabid = 0 (the top level load in a newly open tab and chrome initiated loads (like updates, captive portal pings etc..) and also OCSP requests) respect their Throttleable class when something for the active tab is loading, nothing more.

- one altering timer 300/2000ms that enables read periodically for a short time
- actual read throttling is in nsHttpTransaction::WriteSegments, returns WOULD_BLOCK
- works perfectly for h1
- ...and uncovers bug 1367861 in h2 stream implementation: reading is not stopped and later call to ResumeRead() on the connection assigned to the trans breaks the logic (the connection is very surprisingly the actual http connection - under the session, and not the stream as I would expect)

- When the last 1-3 class transaction is done, there are only "background tab and throttleable" class transactions (downloads, TP listed, whatever unimportant stuff) which we wake after few hundred milliseconds.  This nicely solves the issue mentioned in comment 4.


Left to finish:
- preferences (including a kill switch)

	Patrick McManus [:mcmanus]
	Comment 8 • 7 years ago

Comment on attachment 8871386 [details] [diff] [review]
v1

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

I like that this should work equally for h2 and h1..

connMgr adds two new timers.. I would rather see this info added to state of the existing timer tick it already has. The existing tick is always armed (for a variable amount of time) when the connmgr has open connections.

a bunch of timer constants should probably be prefs to play with ratios.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3108,5 @@
> +    ResumeReadOf(mActiveTransactions[true], true);
> +}
> +
> +void
> +mozilla::net::nsHttpConnectionMgr::ResumeReadOf(

you shouldn't need the mozilla::net

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +840,5 @@
>      }
>  
> +    MOZ_ASSERT(!mReadingStopped, "Should not get WriteSegments when reading is stopped");
> +    if (gHttpHandler->ConnMgr()->ShouldStopReading(this, mThrottleResponse)) {
> +        reentrantFlag = false;

you can remove all this reentrant flag logic as well as the ifdef for 1153929 below (check with valentin to be sure). its not a going concern and is crufting stuff up.

	Honza Bambas (:mayhemer) Assignee
	Comment 9 • 7 years ago

(In reply to Patrick McManus [:mcmanus] from comment #8)
> Comment on attachment 8871386 [details] [diff] [review]
> v1
> 
> Review of attachment 8871386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like that this should work equally for h2 and h1..
> 
> connMgr adds two new timers.. I would rather see this info added to state of
> the existing timer tick it already has. The existing tick is always armed
> (for a variable amount of time) when the connmgr has open connections.

I was thinking of it too, but there is one counter argument: when we cancel the delayed resume timer because some 'higher class' transactions were added in a short time, we want the ticker go on as if nothing would happen - i.e. resume in those original 3 seconds, uninterrupted, unaffected.

To achieve that w/o the second timer I would have to in a relatively complicated way track the time to reschedule the ticker again to keep the regular 3 seconds interval.

Note that timers are relatively cheap and the result would be nearly the same: cancel the ticker, reschedule it to those 300ms.  Then, when it would be about to be canceled (because 'higher trasx' have been added) then it has to be canceled and rescheduled again with an interval to keep the original 3 seconds (need a timestamp for it and also probably some other flags.. and I don't like flags).  This adds, side by the added complexity, one more timer cancel/schedule to the code path, I really don't see a point.  My solution reduces to only adding one more timer with a very clear purpose and lifetime, nothing else.  From the performance point of view it's even better, from the memory point of view it's negligible.

> 
> a bunch of timer constants should probably be prefs to play with ratios.

there are, just was lazy to use them (will update before r?)

> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ +3108,5 @@
> > +    ResumeReadOf(mActiveTransactions[true], true);
> > +}
> > +
> > +void
> > +mozilla::net::nsHttpConnectionMgr::ResumeReadOf(
> 
> you shouldn't need the mozilla::net

Visual studio...

> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +840,5 @@
> >      }
> >  
> > +    MOZ_ASSERT(!mReadingStopped, "Should not get WriteSegments when reading is stopped");
> > +    if (gHttpHandler->ConnMgr()->ShouldStopReading(this, mThrottleResponse)) {
> > +        reentrantFlag = false;
> 
> you can remove all this reentrant flag logic as well as the ifdef for
> 1153929 below (check with valentin to be sure). its not a going concern and
> is crufting stuff up.

Good to know :)

Thanks!

	Honza Bambas (:mayhemer) Assignee
	Comment 10 • 7 years ago

- two times left (comment 9)
- h2 throttling disabled (mActivatedAsH2)
- preferences
- when class of service of the channel is changed, it's carried down to the transaction as well (to the socket thread)
- added log of counts of the active transactions

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b88506e9c1aafbf3fae46a2e64e2426b68c893b3

	Honza Bambas (:mayhemer) Assignee
	Comment 11 • 7 years ago

> - two *timers* left (comment 9)

	Patrick McManus [:mcmanus]
	Comment 12 • 7 years ago

what's up with the new "activatedasH2" code? I thought the point of this was to throttle at the same place (by having the nsHttpTransaction not read).

	Patrick McManus [:mcmanus]
	Comment 13 • 7 years ago

if you could ni the reply, that would really help :)

	Honza Bambas (:mayhemer) Assignee
	Comment 14 • 7 years ago

(In reply to Patrick McManus [:mcmanus] from comment #12)
> what's up with the new "activatedasH2" code? I thought the point of this was
> to throttle at the same place (by having the nsHttpTransaction not read).

It's there because I'd like to land this w/o being blocked by bug 1367861.  That bug will also remove that 'activatedasH2' member and conditions of course.

An h2 transaction breaks when the throttling is involved, hence we would likely break the browser.  Having throttling at least for h1 (where I believe it can have a larger impact) is my goal for 55 (closing to feature freeze).

	Honza Bambas (:mayhemer) Assignee
	Comment 15 • 7 years ago

Thanks for the review Patrick!

few minor tweaks added:
- changed names of the prefs to include ".http" since those are http-only and also we already observe them in the handler
- when pref switched to "on" ensure the ticker, when needed
- timer is not reinstated in ThrottlerTick() when it's not needed

Rather retest once more:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd4e2760386af2d77a6084f41448968deeb8a7a8

	Pulsebot
	Comment 16 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0a62851c33d
Throttling of HTTP transactions. r=mcmanus

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d0a62851c33d