833178 - Increase Stream Transport thread pool max threads

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Doug Turner (:dougt)]

Attachment: Homescreen IO Event latency

I was investigating the wait times of events that get dispatched to the socket transport thread pool.  "Latency" is the time between the event is Dispatch()'ed and the time the event is actually Run().  Currently the socket transport thread pool is configured to have 4 threads.   That is, if you dispatch 5 events, the 5th event will be queued until one of the other 4 events is finished.   We are now using this thread pool for more than just socket IO.  For example, now ipc blob and device storage now use this threadpool for any non-main thread operations they need to do.

Launching the Homescreen on B2G results in a number of events.  The follow graph shows the events and their latency.  What is troubling is that there are some very high wait times on some of the events.

I think we need to get some numbers on what upping the default thread count to something like 20 does to application performance.  I think we also need to figure out if this is a problem for desktop.

Comment 1

[Doug Turner (:dougt)]

*stream* transport service

http://mxr.mozilla.org/mozilla-central/search?string=NS_STREAMTRANSPORTSERVICE_CONTRACTID

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This was discussed in bug 757511 but I guess nobody got around to filing the follow-up bug, which is now this.

I think that until recently, the disk cache was the biggest/primary user of this service. I am not sure if we limited the number of threads used for any particular reason; I suspect, instead, that the limit is very old and we never measured whether increasing it would help.

The most likely negative effect of this is that it may cause disk seek overhead to increase, since there will be more concurrent disk I/O operations executing. For this reason, it seems like it would be a good idea to segregate disk- and non-disk- operations into separate pools, so that we can tune disk seek overhead independently of negative effects on consumers that are using this thread pool for non-disk activities.

Comment 3

[Doug Turner (:dougt)]

on my fast linux box w/ SSD, i never see any latency at all with the current configuration.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.1

bump the threadcount

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 704934 [details] [diff] [review]
patch v.1

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

::: netwerk/base/src/nsStreamTransportService.cpp
@@ +445,4 @@
>      mPool->SetName(NS_LITERAL_CSTRING("StreamTrans"));
> +    mPool->SetThreadLimit(25);
> +    mPool->SetIdleThreadLimit(1);
> +    mPool->SetIdleThreadTimeout(PR_SecondsToInterval(30));

Nit: please put these lines back where they originally were (above the calls to SetName) so that the blame for this file is less insane.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 704934 [details] [diff] [review]
patch v.1

The reason for the change is that I wanted to be able to see who's changing the threadpool thread limit.  If you set the name later, i can't log who's making such a change.  I suppose I can make that change later.

Comment 7

[Doug Turner (:dougt)]

what do you want to do about this line:

nsStreamTransportService::LowerThreadLimit()
...
    if (threadLimit == 4) {
...

Should it be also 25?  I tend to think we should just get rid of it all together.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We could also just not piggyback on stream transport pool and make a new one for our DOM file IO.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Doug Turner (:dougt) from comment #6)
> The reason for the change is that I wanted to be able to see who's changing
> the threadpool thread limit.  If you set the name later, i can't log who's
> making such a change.  I suppose I can make that change later.

OK. Just leave it as you have it.

(In reply to Doug Turner (:dougt) from comment #7)
> what do you want to do about this line:
> 
> nsStreamTransportService::LowerThreadLimit()
> ...
>     if (threadLimit == 4) {
> ...
> 
> Should it be also 25?  I tend to think we should just get rid of it all
> together.

It should also be 25 as you've written the patch. However, I suggest that you just backout the patch for bug 757511 that added that code, as this patch should make it unnecessary. (i.e. I agree that we should get rid of it altogether.)

(In reply to ben turner [:bent] from comment #8)
> We could also just not piggyback on stream transport pool and make a new one
> for our DOM file IO.

I think DOM file IO is a reasonable use of the stream transport service. Probably not worth changing that for B2G 1.0.

I am more concerned about the non-file-I/O uses of stream transport service. I don't think it is a good idea to use the stream transport service's threadpool for non-IO tasks.

Comment 10

[Jason Duell]

Would it be worth making the thread pool count a pref so we can tweak it more easily?  I can imagine we might want less threads on mobile devices then desktop (how much memory do we consume per thread--do we set the stack size or use the OS's default?).  It would also allow us to more easily benchmark which value is optimal here rather than just pulling one out of a hat.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85c0b6224065

Comment 12

[Doug Turner (:dougt)]

jason - sure.  follow up material! ;)

Comment 13

[Andrew Overholt [:overholt]]

Comment on attachment 704934 [details] [diff] [review]
patch v.1

Without information about why this needs to be uplifted, it's hard to make an approval-mozilla-b2g18 decision.  Please re-nom if you'd still like to get this in.

Comment 14

[Andrew Overholt [:overholt]]

Comment on attachment 704934 [details] [diff] [review]
patch v.1

Doug gave some verbal justification for uplifting and it was sufficient IMHO.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/945b0ed70929