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

[Valerie (Yu-Ching) Peng]

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