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

[Anthony Scarpino]

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