675613 - Increase limit for thread number in ssltunnel

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

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).

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Honza Bambas (:mayhemer)]

(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

Comment 3

[(not currently active) Ted Mielczarek]

Okay, then rs=me to just bump that up to a suitably large number.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v2

Limit is 100 threads per server, initially we have 2 threads per server (as before).

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 549821 [details] [diff] [review]
v2

Ah, you said sr=me :) so no bother with reviewing.

Comment 6

[Honza Bambas (:mayhemer)]

(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

[Wan-Teh Chang]

(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

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[(not currently active) Ted Mielczarek]

Note that for mobile testing we run ssltunnel+httpd.js on a desktop OS to serve the test content.

Comment 10

[Honza Bambas (:mayhemer)]

(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.

Comment 11

[Honza Bambas (:mayhemer)]

(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.

Comment 12

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ac99cb76fd86

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/ac99cb76fd86