PSA: Network 'jank' - get your blocking IO off of STS thread!

[Randell Jesup]

As a result of some conversations on IRC, it came to light that a bunch
of blocking IO operations were apparently moved off of MainThread
(presumably to avoid UI 'jank'), and instead Dispatched to
STREAMTRANSPORTSERVICE (aka STS thread).

Some examples pointed out to me: FilePicker, the spell-checker, the
DeviceStorage DOM code, DOM cache code in Manager.cpp (via
BodyStartWriteStream()), even perhaps ResolvedCallback in
ServiceWorkers. (I haven't looked closely at all of the uses yet.)

Good, right? No jank in the UI! No - now the jank is in the network
code!

STS is the primary network thread, and blocking that without a Darn Good
Reason will cause all sorts of negative effects. Page loads, XHR,
WebSockets, WebRTC (which will react by adding delay to cover the
'jitter' in apparent network RTT) and many other things will have
small-to-large negative impacts.

Perhaps some of these listed above (and others) don't block or even have
a legitimate need to run on STS thread - ok, if so, comment as to why
it's ok/needed. Everyone else should not be using STS thread as a
convenient target....

It does point out that with the need to get a lot of blocking operations
off of MainThread, it would help other modules to have a single service
or ThreadPool for dumping such operations to. This would mean less
code, less incorrect/undone thread shutdowns/etc. Thoughts? (Perhaps
such a service could use LazyIdleThreads, which will shut themselves
down after inactivity.)

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[David Rajchenbach-Teller]

Note that we have bug 990804 opened for providing such a service.
Unfortunately, this bug is currently stalled because we apparently
didn't have a clear idea of what API clients would need.

I'm willing to pick this for Q2, if there is interest.

Cheers,
David

David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Benjamin Kelly]

On Thu, Mar 26, 2015 at 2:46 AM, Randell Jesup <rjesu...@jesup.org>
wrote:

> Some examples pointed out to me: FilePicker, the spell-checker, the
> DeviceStorage DOM code, DOM cache code in Manager.cpp (via
> BodyStartWriteStream()), even perhaps ResolvedCallback in
> ServiceWorkers. (I haven't looked closely at all of the uses yet.)
>

Sorry for this. Obviously there has been some confusion as I was explicitly
directed towards STS during the DOM Cache development. (I even thought
there was a separate SocketTransportService which was different from
StreamTransportService.)

In any case, I wrote a bug to fix the Cache issue:

https://bugzilla.mozilla.org/show_bug.cgi?id=1147850

I will try to fix this in the next couple weeks.

Sorry again.

Ben

[Benjamin Kelly]

Actually, I'm going to steal bug 990804 and see if we can get something
worked out now. My plan is just to duplicate the STS code with a different
XPCOM uuid for now.

On Thu, Mar 26, 2015 at 9:29 AM, Benjamin Kelly <bke...@mozilla.com> wrote:

> On Thu, Mar 26, 2015 at 2:46 AM, Randell Jesup <rjesu...@jesup.org>
> wrote:
>

>> Some examples pointed out to me: FilePicker, the spell-checker, the
>> DeviceStorage DOM code, DOM cache code in Manager.cpp (via
>> BodyStartWriteStream()), even perhaps ResolvedCallback in
>> ServiceWorkers. (I haven't looked closely at all of the uses yet.)
>>
>

[Patrick McManus]

thanks bkelly

On Thu, Mar 26, 2015 at 9:01 AM, Benjamin Kelly <bke...@mozilla.com> wrote:

> Actually, I'm going to steal bug 990804 and see if we can get something
> worked out now. My plan is just to duplicate the STS code with a different
> XPCOM uuid for now.
>
> On Thu, Mar 26, 2015 at 9:29 AM, Benjamin Kelly <bke...@mozilla.com>
> wrote:
>
> > On Thu, Mar 26, 2015 at 2:46 AM, Randell Jesup <rjesu...@jesup.org>
> > wrote:
> >

> >> Some examples pointed out to me: FilePicker, the spell-checker, the
> >> DeviceStorage DOM code, DOM cache code in Manager.cpp (via
> >> BodyStartWriteStream()), even perhaps ResolvedCallback in
> >> ServiceWorkers. (I haven't looked closely at all of the uses yet.)
> >>
> >

> > Sorry for this. Obviously there has been some confusion as I was
> > explicitly directed towards STS during the DOM Cache development. (I
> even
> > thought there was a separate SocketTransportService which was different
> > from StreamTransportService.)
> >
> > In any case, I wrote a bug to fix the Cache issue:
> >
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1147850
> >
> > I will try to fix this in the next couple weeks.
> >
> > Sorry again.
> >
> > Ben
> >

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
>

[Benjamin Kelly]

Nathan suggests the following plan to avoid code duplication:

1) Move STS code into xpcom/io under a different name and XPCOM uuid.
2) Make the necko STS a no-op inherit from the xpcom code with a different
uuid.

This would give us two service instances without duplicating any code.

Also, I'll clarify this comment to indicate what the different sservice
instances are for. Right now it kind of suggests people use STS for file
streams:


https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamTransportService.idl#14

Any objections?

Thanks.

Ben

On Thu, Mar 26, 2015 at 10:08 AM, Patrick McManus <mcm...@ducksong.com>
wrote:

> thanks bkelly
>
> On Thu, Mar 26, 2015 at 9:01 AM, Benjamin Kelly <bke...@mozilla.com>
> wrote:
>
>> Actually, I'm going to steal bug 990804 and see if we can get something
>> worked out now. My plan is just to duplicate the STS code with a
>> different
>> XPCOM uuid for now.
>>
>> On Thu, Mar 26, 2015 at 9:29 AM, Benjamin Kelly <bke...@mozilla.com>
>> wrote:
>>
>> > On Thu, Mar 26, 2015 at 2:46 AM, Randell Jesup <rjesu...@jesup.org>
>> > wrote:
>> >

>> >> Some examples pointed out to me: FilePicker, the spell-checker, the
>> >> DeviceStorage DOM code, DOM cache code in Manager.cpp (via
>> >> BodyStartWriteStream()), even perhaps ResolvedCallback in
>> >> ServiceWorkers. (I haven't looked closely at all of the uses yet.)
>> >>
>> >

[Patrick McManus]

:

> On Thu, Mar 26, 2015 at 2:46 AM, Randell Jesup <rjesu...@jesup.org>
> wrote:
>


> t. (I even thought
> there was a separate SocketTransportService which was different from
> StreamTransportService.)
>
>

You're right they are different things.

The socket transport service is a single thread that does most of the low
level networking -
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransportService2.cpp#487
.. blocking this thread would be very bad.

and the stream transport service is a thread pool that is used for
buffering management primarily, etc..
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsStreamTransportService.cpp#485
.. I'm not sure how I feel about overloading arbitrary other functionality
here, but its certainly less damaging than blocking the single socket
thread.

Is this thread mostly just confusion from these things sounding so much
alike? Or am I confused now?

[Kyle Huey]

On Thu, Mar 26, 2015 at 7:49 AM, Patrick McManus <mcm...@ducksong.com>
wrote:

> Is this thread mostly just confusion from these things sounding so much
> alike? Or am I confused now?
>

Most likely.

Does anyone have actual data to show that this is a problem?

- Kyle

[Ehsan Akhgari]

There's some truth to it. Looks like some code uses the *socket*
transport service when it probably means *stream* transport service.
Example:
<http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#249>

But other examples such as DOM Cache are not affected as far as I can tell.

[Patrick McManus]

good catch.. looking at
https://mxr.mozilla.org/mozilla-central/ident?i=NS_SOCKETTRANSPORTSERVICE_CONTRACTID
the only uses I see other than the one Ehsan unearthed are expected.. so
maybe that's the sum of the short term work.

On Thu, Mar 26, 2015 at 10:06 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

[Ehsan Akhgari]

On 2015-03-26 11:14 AM, Patrick McManus wrote:
> good catch.. looking at
> https://mxr.mozilla.org/mozilla-central/ident?i=NS_SOCKETTRANSPORTSERVICE_CONTRACTID
> the only uses I see other than the one Ehsan unearthed are expected.. so
> maybe that's the sum of the short term work.

Great! Filed <https://bugzilla.mozilla.org/show_bug.cgi?id=1147913>

[Kyle Huey]

Can we stop exposing the socket transport service's nsIEventTarget outside
of Necko?

- Kyle

On Thu, Mar 26, 2015 at 8:14 AM, Patrick McManus <mcm...@ducksong.com>

wrote:

> good catch.. looking at
> https://mxr.mozilla.org/mozilla-central/ident?i=NS_SOCKETTRANSPORTSERVICE_CONTRACTID
> the only uses I see other than the one Ehsan unearthed are expected.. so
> maybe that's the sum of the short term work.
>

[Patrick McManus]

media uses it by agreement and in an appropriate way to support rtcweb.

[Honza Bambas]

One more place:

https://bugzilla.mozilla.org/show_bug.cgi?id=1147921

-hb-

On 3/26/2015 16:14, Patrick McManus wrote:
> good catch.. looking at
> https://mxr.mozilla.org/mozilla-central/ident?i=NS_SOCKETTRANSPORTSERVICE_CONTRACTID
> the only uses I see other than the one Ehsan unearthed are expected.. so
> maybe that's the sum of the short term work.
>
> On Thu, Mar 26, 2015 at 10:06 AM, Ehsan Akhgari <ehsan....@gmail.com>
> wrote:
>
>> On 2015-03-26 11:00 AM, Kyle Huey wrote:
>>
>>> On Thu, Mar 26, 2015 at 7:49 AM, Patrick McManus <mcm...@ducksong.com>
>>> wrote:
>>>
>>> Is this thread mostly just confusion from these things sounding so much
>>>> alike? Or am I confused now?
>>>>
>>>>
>>> Most likely.
>>>
>>> Does anyone have actual data to show that this is a problem?
>>>
>> There's some truth to it. Looks like some code uses the *socket*
>> transport service when it probably means *stream* transport service.
>> Example: <http://mxr.mozilla.org/mozilla-central/source/dom/
>> workers/ServiceWorkerEvents.cpp#249>
>>
>> But other examples such as DOM Cache are not affected as far as I can tell.
>>
>>

[Randell Jesup]

>Can we stop exposing the socket transport service's nsIEventTarget outside
>of Necko?

If we move media/mtransport to necko... or make an exception for it (and
dom/network/UDPSocket and TCPSocket, etc). Things that remove loaded
footguns (or at least lock them down) are good.

Glad the major real problem was too-similar-names (I'd never heard of
STREAMTRANSPORTSERVICE (or if I had, it had been long-forgotten, or
mis-read as SOCKETTRANSPORTSERVICE)).

[Bobby Holley]

dom/media has a very useful system called SharedThreadPool that gives you
static on-demand access to a named (and refcounted) threadpool. Probably
just moving that to XPCOM and then coming up with a naming convention
("MiscThreadPool" or something) might solve this problem.

On Thu, Mar 26, 2015 at 8:37 AM, Randell Jesup <rjesu...@jesup.org>
wrote:

[Shih-Chiang Chien]

dom/network/UDPSocket doesn't use SocketTransportService directly so it
doesn't create exception. It should be ok to leave it outside of Necko.

Best Regards,
Shih-Chiang Chien
Mozilla Taiwan

On Thu, Mar 26, 2015 at 11:37 PM, Randell Jesup <rjesu...@jesup.org>
wrote:

[Cameron Kaiser]

On 3/26/15 8:37 AM, Randell Jesup wrote:
>> Can we stop exposing the socket transport service's nsIEventTarget outside
>> of Necko?
>
> If we move media/mtransport to necko... or make an exception for it (and
> dom/network/UDPSocket and TCPSocket, etc). Things that remove loaded
> footguns (or at least lock them down) are good.
>
> Glad the major real problem was too-similar-names (I'd never heard of
> STREAMTRANSPORTSERVICE (or if I had, it had been long-forgotten, or
> mis-read as SOCKETTRANSPORTSERVICE)).
>

The OverbiteFF (gopher and legacy protocols) add-on uses
nsISocketTransportService to open sockets, and I'm sure it's not the
only one that does. The implementation is non-blocking, but I want to
clarify from the above post that the intention is not to block non-Necko
consumers from using it. Is this acceptable usage, or is it deprecated?

Cameron Kaiser

[Patrick McManus]

it sounds like overbite is using it as intended.