One theory why throttling has a poor effect is that suspending a channel and the trans pump will not push enough on the socket, since copying the data to the pipe still continues.

Nick, would you be willing to work on this? If not, bounce back please. Thanks.

I don't have the cycles for this at the moment.

this is a cdp bug. (57). let's not toss it into next :)

Nick, since you are the author of the throttling service, it would be logical for you to work on this one too. Do you have time now? This is important to actually finish the whole throttling feature. If not, bounce again back to me. Thanks.

Sorry, still super-swamped.

- nsAHttpConnection::ThrottleResponse(bool) - h1 (nsHttpConnection) simply calls sock-instream.asyncWait only for closure - h2 (Http2Session) is a no-op - nsHttpTransaction::ThrottleResponse(bool) simply forwards to its conn or remembers to do so when assigned a conn - nsHttpChannel::Suspend/ResumeInternal calls it's transaction ThrottleResponse when suspend count goes from or to zero respectively - nsHttpChannel also added a reaction to adding the class later during the channel lifetime, which is the case for downloads (a different bug to use this for downloads is filed and CDP blocking) - changing defaults for throttling to 3000/200 ms of suspend/resume time Made some tests with 3 running downloads and [1] on my ADSL conn and there is a huge difference. 20-25s cnn.com load vs 10-11s. This was the missing piece! Note that in wireshark it's immediately visible we send back a full-win update response and the next router immediately stops sending. Also watching a periodic ping to some nearby server, it's apparently better when throttling is on. [1] https://bugzilla.mozilla.org/attachment.cgi?id=8826861

Comment on attachment 8856120 [details] [diff] [review] v1 Review of attachment 8856120 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +6377,5 @@ > //----------------------------------------------------------------------------- > // HttpChannel::nsIClassOfService > //----------------------------------------------------------------------------- > + > +void net::nsHttpChannel::OnClassOfServiceUpdated() (Sorry for the drive-by)... should we also add/remove this channel from the throttling service in here?

(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #7) > Comment on attachment 8856120 [details] [diff] [review] > v1 > > Review of attachment 8856120 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +6377,5 @@ > > //----------------------------------------------------------------------------- > > // HttpChannel::nsIClassOfService > > //----------------------------------------------------------------------------- > > + > > +void net::nsHttpChannel::OnClassOfServiceUpdated() > > (Sorry for the drive-by)... should we also add/remove this channel from the > throttling service in here? Yes and it will also change the patch in bug 1348062 as you mentioned during the review. I will update both patches. Thanks.

comment 8 seems to be a small enough change that I'm going to review this now.. comment if I should stop :)

Comment on attachment 8856120 [details] [diff] [review] v1 Review of attachment 8856120 [details] [diff] [review]: ----------------------------------------------------------------- let me know what you think about doing a thread dispatch at the beginning rather than trying to do this with locking. I don't think there is any reason it has to be synchronous. other than that lgtm; thanks! is there a bug # for a h2 version (which can accomplish this by not reading the stream rather than the socket). h2 is a huge fraction of our traffic. ::: modules/libpref/init/all.js @@ +2002,5 @@ > pref("network.auth.private-browsing-sso", false); > > // Control how the throttling service works - number of ms that each > // suspend and resume period lasts (prefs named appropriately) > +pref("network.throttle.suspend-for", 3000); these prefs also have defaults in ThrottlingService.cpp that it makes sense to have match.. if you don't mind a couple comments in that cpp on what the suspend and resume params do from a high level would help too. ::: netwerk/protocol/http/nsHttpConnection.cpp @@ +1402,5 @@ > + if (mResponseThrottled || mResumeRecvOnUnthrottle) { > + mResumeRecvOnUnthrottle = true; > + > + if (mSocketIn) { > + return mSocketIn->AsyncWait(this, definitely LOG(()) here :) @@ +2104,5 @@ > return NS_OK; > } > > +void nsHttpConnection::ThrottleResponse(bool aThrottle) > +{ I would greatly prefer that this method assert the socket thread and that a connection manager interfrace for throttleResponse be added that dispatches to the socket thread to run this (rather than the channel calling into the connection while the connection is running) @@ +2105,5 @@ > } > > +void nsHttpConnection::ThrottleResponse(bool aThrottle) > +{ > + if (aThrottle) { LOG()

(In reply to Patrick McManus [:mcmanus] from comment #10) > Comment on attachment 8856120 [details] [diff] [review] > v1 > > Review of attachment 8856120 [details] [diff] [review]: > ----------------------------------------------------------------- > > let me know what you think about doing a thread dispatch at the beginning > rather than trying to do this with locking. I don't think there is any > reason it has to be synchronous. You mean lock acq at [1] - nsHttpTransaction::ThrottleResponse? Hmm.. accessing the mConnection member is always locked, as I understand the code. So I think this is needed, but we could at least call mConnection->ThrottleResponse on a local refptr unlocked. Or did you have something else in mind? > > other than that lgtm; thanks! > > is there a bug # for a h2 version (which can accomplish this by not reading > the stream rather than the socket). h2 is a huge fraction of our traffic. Why did I live in persuasion that suspending a channel (transactions) is enough for h2? I though something like that has already been implemented as a "nice feature of h2". I will file that bug. > > ::: modules/libpref/init/all.js > @@ +2002,5 @@ > > pref("network.auth.private-browsing-sso", false); > > > > // Control how the throttling service works - number of ms that each > > // suspend and resume period lasts (prefs named appropriately) > > +pref("network.throttle.suspend-for", 3000); > > these prefs also have defaults in ThrottlingService.cpp that it makes sense > to have match.. if you don't mind a couple comments in that cpp on what the > suspend and resume params do from a high level would help too. Oh, forgot about them, will update + comms. > > ::: netwerk/protocol/http/nsHttpConnection.cpp > @@ +1402,5 @@ > > + if (mResponseThrottled || mResumeRecvOnUnthrottle) { > > + mResumeRecvOnUnthrottle = true; > > + > > + if (mSocketIn) { > > + return mSocketIn->AsyncWait(this, > > definitely LOG(()) here :) Yes, already added locally :D > > @@ +2104,5 @@ > > return NS_OK; > > } > > > > +void nsHttpConnection::ThrottleResponse(bool aThrottle) > > +{ > > I would greatly prefer that this method assert the socket thread and that a > connection manager interfrace for throttleResponse be added that dispatches > to the socket thread to run this (rather than the channel calling into the > connection while the connection is running) OK. > > @@ +2105,5 @@ > > } > > > > +void nsHttpConnection::ThrottleResponse(bool aThrottle) > > +{ > > + if (aThrottle) { > > LOG() Done as well :) [1] https://bugzilla.mozilla.org/attachment.cgi?id=8856120&action=diff#a/netwerk/protocol/http/nsHttpTransaction.cpp_sec3

(In reply to Patrick McManus [:mcmanus] from comment #10) > > +void nsHttpConnection::ThrottleResponse(bool aThrottle) > > +{ > > I would greatly prefer that this method assert the socket thread and that a > connection manager interfrace for throttleResponse be added that dispatches > to the socket thread to run this (rather than the channel calling into the > connection while the connection is running) > To be honest I'm not sure what you mean with this. Can you be more specific please? > let me know what you think about doing a thread dispatch at the beginning > rather than trying to do this with locking. I don't think there is any > reason it has to be synchronous. And to make sure this one is not lost, here I'm not sure as well.

I'm suggesting all of this code be socket-thread-only and that when the main thread needs to do something it posts an event to the socket thread asynchronously. so instead of mTransaction->ThrottleResponse() on the main thread, you do something like gHttpHandler->ThrottleResponse(mTransaction) similar to RescheduleTransaction that should make all the locking go away.

(In reply to Patrick McManus [:mcmanus] from comment #13) > I'm suggesting all of this code be socket-thread-only and that when the main > thread needs to do something it posts an event to the socket thread > asynchronously. > > so instead of mTransaction->ThrottleResponse() on the main thread, you do > something like gHttpHandler->ThrottleResponse(mTransaction) similar to > RescheduleTransaction > > that should make all the locking go away. Got it. Thanks.

Comment on attachment 8857525 [details] [diff] [review] v2 Review of attachment 8857525 [details] [diff] [review]: ----------------------------------------------------------------- thanks!

Comment on attachment 8857525 [details] [diff] [review] v2 better: https://treeherder.mozilla.org/#/jobs?repo=try& revision=f448ab3b6386ea53e62eed3bac9f001860b0c39c

One small change that probably doesn't need a review round: diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -8513,20 +8513,17 @@ nsHttpChannel::ResumeInternal() NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED); LOG(("nsHttpChannel::ResumeInternal [this=%p]\n", this)); if (--mSuspendCount == 0) { mSuspendTotalTime += (TimeStamp::NowLoRes() - mSuspendTimestamp). ToMilliseconds(); - // Do this regardless the class, since we don't want to get - // stuck forever when someone unsets the class during the suspend - // period. - if (mTransaction) { + if (mClassOfService & nsIClassOfService::Throttleable && mTransaction) { gHttpHandler->ThrottleTransaction(mTransaction, false); } if (mCallOnResume) { nsresult rv = AsyncCall(mCallOnResume); mCallOnResume = nullptr; NS_ENSURE_SUCCESS(rv, rv); } We can do it this way, more optimally (only when the class is set on a channel,) since if it happens to be unset during the suspend period, the transaction will be unthrottled in OnClassOfServiceUpdated. Rather re-try'ed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ecea12802ac5eed97b9d092887ccf5068994a0d

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/74d41acc175e When an HTTP channel is throttled, stop reading from the socket when using HTTP/1. r=mcmanus