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

[Anthony Scarpino]

On 03/06/2014 11:24 AM, Valerie (Yu-Ching) Peng wrote:
> Tony,
>
> I like this version better - a local "instance" session pool for
> P11Cipher is a bit excessive I think.
>
> One thing that I noticed with this change is that by switching to the
> ConcurrentLinkedQueue, you are retrieving sessions from the head and put
> them back at the end. This is different from the original approach of
> first-in-last-out. The subtlety in this is that it will take longer in
> order for sessions to pass their MAX_IDLE_TIME and be closed to shrink
> the pool size down.

I didn't feel this was a big expense.  Yes it does pull the oldest 
session first, but my thinking was the queue was to handle surges in 
usage. A possible extra session or two wasn't a big deal.  If you prefer 
me to go to the original method, I can change their patch to use 
ConcurrentLinkedDeque, which does allow push and pop from both ends of 
the queue

> What is the synchronized(this) call for in the various debug code?
> I am still going through the rest of SessionManager.java file but should
> finish today.

Yes, two of those seem useless, the last one does set maxActiveSessions, 
so I should leave that in.


> Thanks,
> Valerie
>
> On 02/28/14 15:03, Anthony Scarpino wrote:
>> So I ran a stress test creating a number of keys and did not see much
>> different memory wise.  But what I did notice that the performance was
>> dependent on the number Cipher.init() calls.  If each thread only has
>> one Cipher.init() call, it performed 5x better (170k vs 844k ops/m),
>> but as soon as you add any additional Cipher.init() calls the
>> performance dropped to equal without this change.
>>
>> When I changed the code to remove the local pool in P11Cipher, the
>> performance was lower, but still 4.5x faster (170k vs 768k ops/m).
>>
>> While there is some performance on the table from an performance
>> test/non-real world standpoint, it does keep session management in
>> SessionManager.  I see value doing it this way given as it's an update
>> to the modern concurrent class rather than old List with synchronized
>> methods.  Valerie, would be more comfortable with the solution?
>>
>> http://cr.openjdk.java.net/~ascarpino/7107611/webrev.02/
>>
>> Tony
>>
>> On 02/21/2014 02:38 PM, Valerie (Yu-Ching) Peng wrote:
>>>
>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>