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

[Anthony Scarpino]

On 02/21/2014 02:38 PM, Valerie (Yu-Ching) Peng wrote:
>
> Where can I find the updated webrev?

Oops.. thought I included it:
http://cr.openjdk.java.net/~ascarpino/7107611/webrev.01/

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