Code Review Request: 7107611 sun.security.pkcs11.SessionManager is scalability blocker

Fri Mar 7 00:12:56 UTC 2014

Last time when I was debugging/observing the session pools, it's not 
just one or two sessions in the queue. It's at least hundreds possibly 
thousands when I ran one of the SSL stress tests.

1) line 273, do you really mean to return when the 
oldestSession.isLive() returns false??
2) line 275 is changed to "return". Wouldn't this mean that the debug 
statement on line 282-285 will rarely if ever be executed?

I think the main performance boost that we gained here is because we 
switched from a synchronize everything approach to a very 
fine-grained-synchronization one. The concern that I have is that is 
whether we need additional synchronization will be required as there are 
several calls involving the "pool" field in the Pool.release(Session) 
method which have no additional synchronization involved... This seems a 
bit too loose?

Regards,
Valerie

On 03/06/14 12:59, Anthony Scarpino wrote:
> On 03/06/2014 11:24 AM, Valerie (Yu-Ching) Peng wrote:
>> Tony,
>>
>> I like this version better - a local "instance" session pool for
>> P11Cipher is a bit excessive I think.
>>
>> One thing that I noticed with this change is that by switching to the
>> ConcurrentLinkedQueue, you are retrieving sessions from the head and put
>> them back at the end. This is different from the original approach of
>> first-in-last-out. The subtlety in this is that it will take longer in
>> order for sessions to pass their MAX_IDLE_TIME and be closed to shrink
>> the pool size down.
>
> I didn't feel this was a big expense.  Yes it does pull the oldest 
> session first, but my thinking was the queue was to handle surges in 
> usage. A possible extra session or two wasn't a big deal.  If you 
> prefer me to go to the original method, I can change their patch to 
> use ConcurrentLinkedDeque, which does allow push and pop from both 
> ends of the queue
>
>> What is the synchronized(this) call for in the various debug code?
>> I am still going through the rest of SessionManager.java file but should
>> finish today.
>
> Yes, two of those seem useless, the last one does set 
> maxActiveSessions, so I should leave that in.
>
>
>> Thanks,
>> Valerie
>>
>> On 02/28/14 15:03, Anthony Scarpino wrote:
>>> So I ran a stress test creating a number of keys and did not see much
>>> different memory wise.  But what I did notice that the performance was
>>> dependent on the number Cipher.init() calls.  If each thread only has
>>> one Cipher.init() call, it performed 5x better (170k vs 844k ops/m),
>>> but as soon as you add any additional Cipher.init() calls the
>>> performance dropped to equal without this change.
>>>
>>> When I changed the code to remove the local pool in P11Cipher, the
>>> performance was lower, but still 4.5x faster (170k vs 768k ops/m).
>>>
>>> While there is some performance on the table from an performance
>>> test/non-real world standpoint, it does keep session management in
>>> SessionManager.  I see value doing it this way given as it's an update
>>> to the modern concurrent class rather than old List with synchronized
>>> methods.  Valerie, would be more comfortable with the solution?
>>>
>>> http://cr.openjdk.java.net/~ascarpino/7107611/webrev.02/
>>>
>>> Tony
>>>
>>> On 02/21/2014 02:38 PM, Valerie (Yu-Ching) Peng wrote:
>>>>
>>>> Where can I find the updated webrev?
>>>>
>>>> As for the performance test, I think we can probably try a 
>>>> "worse-case"
>>>> (say 5000 Cipher objects which we don't re-use, just create, do
>>>> operation, and then discard) and "best-case" (same # of objects, but
>>>> re-use) against both impls and see how much a difference we'd observe.
>>>>
>>>> Thanks,
>>>> Valerie
>>>>
>>>> On 02/13/14 11:16, Anthony Scarpino wrote:
>>>>> On 02/11/2014 02:18 PM, Valerie (Yu-Ching) Peng wrote:
>>>>>>
>>>>>> Please see comments inline.
>>>>>>
>>>>>> On 02/10/14 16:30, Anthony Scarpino wrote:
>>>>>>> On 02/10/2014 03:04 PM, Valerie (Yu-Ching) Peng wrote:
>>>>>>>>
>>>>>>>> Well, there are some changes that look strange which I need more
>>>>>>>> time to
>>>>>>>> figure out.
>>>>>>>> For example, the purpose for the 2 new pkg private methods of
>>>>>>>> poll(Pool)
>>>>>>>> and release(Pool, Session) of the static class Pool.
>>>>>>>
>>>>>>> New methods are for supporting local pools.
>>>>>>
>>>>>> Yes, I can tell that they are for supporting the local pools, but 
>>>>>> are
>>>>>> they really necessary and why do they belong in class Pool as 
>>>>>> instance
>>>>>> methods?
>>>>>> For example, the Pool.release(Pool, Session) method seems
>>>>>> redundant, the
>>>>>> caller can simply just call the release method on the cachePool
>>>>>> argument
>>>>>> as it has nothing to do with the Pool object that it invokes upon.
>>>>>> Secondly, the poll method also seems strange as the cachePool is 
>>>>>> used
>>>>>> first and the actual Pool object is only acting as backup. It's
>>>>>> kind of
>>>>>> ambiguous on what it is expected to do unless you looked at its
>>>>>> implementation.
>>>>>
>>>>> I didn't notice that these methods aren't even called in addition to
>>>>> being pointless like you say.  I'm removing them
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Also, now that every P11Cipher "object" has its own session pool,
>>>>>>>> will
>>>>>>>> this have any impact on the overall memory usage.
>>>>>>>
>>>>>>> I would think the memory usage would mostly transfer the session 
>>>>>>> from
>>>>>>> SessionManager to P11Cipher.
>>>>>>
>>>>>> The session object itself may be transferred and does not matter, 
>>>>>> but
>>>>>> the supporting structure, e.g. esp. the queue, is now present in 
>>>>>> every
>>>>>> P11Cipher object. How much impact this has will depend on how
>>>>>> List/ConcurrentLinkedQueue is implemented as well as the P11Cipher
>>>>>> usages in the calling application. It's essential to verify that 
>>>>>> this
>>>>>> change has NO significant impact on other apps such as some JSSE 
>>>>>> apps.
>>>>>
>>>>> Is there a perf test you'd recommend to try this?
>>>>>
>>>>>> Valerie
>>>>>>> It is true that if many P11Cipher objects are created, that 
>>>>>>> could be
>>>>>>> more idle op session around if P11Cipher objects lingers.  But the
>>>>>>> object session stay in SessionManager, which is probably the 
>>>>>>> bulk of
>>>>>>> the memory usage.
>>>>>>>
>>>>>>>> If P11Cipher object is not re-used much and only have a short life
>>>>>>>> span,
>>>>>>>> what's the point of having a local session pool to re-use the
>>>>>>>> sessions.
>>>>>>>
>>>>>>> If the P11Cipher object is created sparingly and short lived,
>>>>>>> creating
>>>>>>> one session for usage and then getting gc'ed, I would suspect 
>>>>>>> crypto
>>>>>>> performance isn't a top issue.  But I could see that only
>>>>>>> happening in
>>>>>>> large crypto deployments using that methodology; however, I'm not
>>>>>>> sure
>>>>>>> that is wise development methodology to start with.  You'd better
>>>>>>> insight if such designs are common.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Tony
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Valerie
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 02/03/14 10:24, Anthony Scarpino wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I'm requesting a review of these changes.  It is mostly from the
>>>>>>>>> patch
>>>>>>>>> that Intel provided, but I have made some cosmetic changes.  The
>>>>>>>>> changes significantly improve performance in multithreaded
>>>>>>>>> application
>>>>>>>>> using pkcs11.
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~ascarpino/7107611/webrev.00/
>>>>>>>>>
>>>>>>>>> thanks
>>>>>>>>>
>>>>>>>>> Tony
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>