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

[Anthony Scarpino]

I've updated the webrev, removing the unneeded methods, and also adding 
back some rather important pool cleanup code I hadn't noticed the patch 
removed.

http://cr.openjdk.java.net/~ascarpino/7107611/webrev.01/

Tony


On 02/13/2014 11:16 AM, 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
>>>>
>>>
>>
>