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

[Valerie (Yu-Ching) Peng]

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.

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

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