Closed
Bug 675613
Opened 13 years ago
Closed 13 years ago
Increase limit for thread number in ssltunnel
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla8
People
(Reporter: mayhemer, Assigned: mayhemer)
References
(Blocks 1 open bug)
Details
(Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
As part of the work to make httpd.js support keep-alive connections we need to increase the limit on number of threads in the pool for ssltunnel, which is using a new thread for each new connection handling. While all connections were immediately closed, threads were quickly released and new ones could be started to handle very soon. But while we now keep connections alive, then after we reach the thread limit, new incoming connections are accepted but starting threads with PR_QueueJob to handle them is blocked until an active thread gets closed (until an idle connection closes).
Attachment #549777 -
Flags: review?(ted.mielczarek)
Comment 1•13 years ago
|
||
Comment on attachment 549777 [details] [diff] [review] v1 Hrm. Can we dynamically adjust the size of the threadpool if we start using up all the threads? Might be nicer than hardcoding a larger value.
Attachment #549777 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 2•13 years ago
|
||
(In reply to comment #1) > Hrm. Can we dynamically adjust the size of the threadpool if we start using > up all the threads? Might be nicer than hardcoding a larger value. According to PR_QueueJob doc [1] looks like we don't. However, according the implementation, it looks like the max number is not responsible for any hard memory allocation. It is just and only used to check the current number of threads. Threads are in a linked list. So, we might set max to some large reasonable number (around 100 ?) and be OK. A new patch? [1] http://www.mozilla.org/projects/nspr/reference/html/prtpool.html#PR_CreateThreadPool
Assignee | ||
Comment 4•13 years ago
|
||
Limit is 100 threads per server, initially we have 2 threads per server (as before).
Attachment #549777 -
Attachment is obsolete: true
Attachment #549821 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 5•13 years ago
|
||
Comment on attachment 549821 [details] [diff] [review] v2 Ah, you said sr=me :) so no bother with reviewing.
Attachment #549821 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #4) > Created attachment 549821 [details] [diff] [review] [diff] [details] [review] > v2 > > Limit is 100 threads per server, initially we have 2 threads per server (as > before). Rather adding some mobile people to check this won't go behind any mobile tests limits.
Comment 7•13 years ago
|
||
(In reply to comment #2) Honza: you cc'ed me without asking me a question. I don't want to guess what you wanted to ask me. It seems that you have found the answer, right?
Comment 8•13 years ago
|
||
mobile does set some "connectios per server" preferences lower than desktop: network.http.max-connections: 6 on mobile (256 on desktop) network.http.max-connections-per-server: 4 on mobile (15 on desktop) network.http.max-persistent-connections-per-server: 4 on mobile (6 on desktop) network.http.max-persistent-connections-per-proxy: 4 on mobile (8 on desktop) Do any of these affect your patch? The striking difference in "network.http.max-connections" gives me pause to ask "why?"
Comment 9•13 years ago
|
||
Note that for mobile testing we run ssltunnel+httpd.js on a desktop OS to serve the test content.
Assignee | ||
Comment 10•13 years ago
|
||
(In reply to comment #7) > (In reply to comment #2) > Honza: you cc'ed me without asking me a question. I don't want to guess > what you wanted to ask me. It seems that you have found the answer, > right? Yes, sorry, I forgot to remove you. Thanks.
Assignee | ||
Comment 11•13 years ago
|
||
(In reply to comment #8) > Do any of these affect your patch? The lower the better here, so the answer is probably "no". As I count, the thread limit should be based as MIN of total connections limit and the limit of persistent connections per hosts (6/4) * the number of virtual https hosts configured + at least one slot per host reserved for a non-persistent connection. For desktop there is a potential for 6*22+22=154 threads. For mobile it is simply 6. Maybe we should make the limit for the number of threads even larger. Default timeout value for a keep-alive connection is planed to be 120 seconds from the server side and is 115 seconds from the client (desktop Fx) side. So, connections might persist and block the thread pool for a long time. Since we run the server on a dedicated desktop, we should not bother that much about the thread limit. I think it is OK to raise the limit in future even higher.
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac99cb76fd86
Whiteboard: [inbound]
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/ac99cb76fd86
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•