757511 - Make it possible to raise the maximum number of threads

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

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

The maximum number of threads for what?

Comment 2

[Jan Varga [:janv]]

sorry, I mean the necko thread pool (stream transport service)

https://bugzilla.mozilla.org/show_bug.cgi?id=726593#c48

will attach a patch once I get results from try

Comment 3

[Jan Varga [:janv]]

Attachment: patch

Comment 4

[Jan Varga [:janv]]

This is needed for bug 726593.

Comment 5

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

Jan, I have several questions:

1. Why is it important to increase the thread limit? Did we find a performance problem that is solved by allowing exactly one more thread? It seems strange to me that four would be not enough but five would be just right.

2. Why increase the thread limit but not the idle thread limit?

3. Why not simply increase the maximum by 1, like so:

    mPool = do_CreateInstance(NS_THREADPOOL_CONTRACTID);
    NS_ENSURE_STATE(mPool);

    // Configure the pool
-   mPool->SetThreadLimit(4);
+   mPool->SetThreadLimit(5);
    mPool->SetIdleThreadLimit(1);
    mPool->SetIdleThreadTimeout(PR_SecondsToInterval(60));

Comment 6

[Jan Varga [:janv]]

Well, originally I wanted to create a new thread pool for FileHandle, but I figured out that we can reuse necko thread pool for it. (Each new thread allocates memory for stack, etc.)
I discussed about it with dougt and we think that this can starve necko thread pool somehow. DeviceStorage will use FileHandle too (besides IndexedDB).

I agree that we might consider to increase the idle thread limit too (in the same method).

However, increasing the thread limit globally would affect all clients and possibly would cause memory footprint increase. So I decided to do it just in case there's a request for FileHandle IO

Comment 7

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

I am not sure how much increasing the limit helps. In theory, if you have N FileHandles, couldn't they all be blocked on I/O at the same time, starving Necko and other Stream Transport Service users, regardless of whether N is 4 or 5? Or, do you serialize all the FileHandle I/O operations so that there's only ever one stream thread being used by the FileHandle service at any one time?

Either way, I don't see increasing--even doubling--the default limit will be a problem; when the threads aren't being used, they will get stopped since the idle limit is one.

Consider this code I added to the SSL implementation recently:

  (void) gCertVerificationThreadPool->SetIdleThreadLimit(5);
  (void) gCertVerificationThreadPool->SetThreadLimit(5);

That creates a whole separate thread pool and those threads never get stopped. I did not notice any significant increase in memory usage when I added that. Also, the functions that the SSL verification threads execute actually use a significant amount of stack memory (at least according to Chromium devs). AFAICT, the stream transport service threads could be set to have a very small stack size if stack size were an issue.

Comment 8

[Jan Varga [:janv]]

(In reply to Brian Smith (:bsmith) from comment #7)
> I am not sure how much increasing the limit helps. In theory, if you have N
> FileHandles, couldn't they all be blocked on I/O at the same time, starving
> Necko and other Stream Transport Service users, regardless of whether N is 4
> or 5? Or, do you serialize all the FileHandle I/O operations so that there's
> only ever one stream thread being used by the FileHandle service at any one
> time?

the former, so yeah, it's still possible to starve Necko, etc.

> 
> Either way, I don't see increasing--even doubling--the default limit will be
> a problem; when the threads aren't being used, they will get stopped since
> the idle limit is one.
> 
> Consider this code I added to the SSL implementation recently:
> 
>   (void) gCertVerificationThreadPool->SetIdleThreadLimit(5);
>   (void) gCertVerificationThreadPool->SetThreadLimit(5);
> 
> That creates a whole separate thread pool and those threads never get
> stopped. I did not notice any significant increase in memory usage when I
> added that. Also, the functions that the SSL verification threads execute
> actually use a significant amount of stack memory (at least according to
> Chromium devs). AFAICT, the stream transport service threads could be set to
> have a very small stack size if stack size were an issue.

If that's the case then I'll file a new bug for increasing the maximum globally (and revert changes made by this bug)

or we can create a separate thread pool too

anyway, I guess that typical hardware can't handle too many I/O operations at the same time, so I would suggest to just increase the current maximum to 5 or 6

Comment 9

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

(In reply to Jan Varga [:janv] from comment #8)
> If that's the case then I'll file a new bug for increasing the maximum
> globally (and revert changes made by this bug)

That's bug 833178 now.

> anyway, I guess that typical hardware can't handle too many I/O operations
> at the same time, so I would suggest to just increase the current maximum to
> 5 or 6

Good to discuss this in bug 833178.

Is this WONTFIX, given bug 833178?

Comment 10

[Doug Turner (:dougt)]

This patch looked like it landed already?

http://hg.mozilla.org/mozilla-central/rev/36e938e51481

Also, i really don't understand this line:
http://hg.mozilla.org/mozilla-central/rev/36e938e51481#l2.32

Comment 11

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

(In reply to Doug Turner (:dougt) from comment #10)
> This patch looked like it landed already?
> 
> http://hg.mozilla.org/mozilla-central/rev/36e938e51481

Surprise!

> Also, i really don't understand this line:
> http://hg.mozilla.org/mozilla-central/rev/36e938e51481#l2.32

The initial limit is 4, so if we try to decrease it below 4, we know that we've tried to decrease it more than we ever tried to increase it.

I am marking this RESOLVED FIXED since the r+d patch was checked in.

IMO, the patch for bug 833178 should backout the patch for this bug.

Comment 12

[Jan Varga [:janv]]

ok, thanks for closing this