Closed Bug 757511 Opened 10 years ago Closed 9 years ago

Make it possible to raise the maximum number of threads

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
The maximum number of threads for what?
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
Attached patch patchSplinter Review
Attachment #626312 - Flags: review?(cbiesinger)
This is needed for bug 726593.
Attachment #626312 - Flags: review?(cbiesinger) → review+
Blocks: 726593
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));
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
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.
(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
(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?
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
(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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
ok, thanks for closing this
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.