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

Valerie (Yu-Ching) Peng valerie.peng at oracle.com
Fri Mar 7 23:15:16 UTC 2014 

line 280 still has "mgr.activeSessions", shouldn't it be 
"mgr.activeSessions.get()"?
That's all.

Thanks,
Valerie

On 03/06/14 19:21, Anthony Scarpino wrote:
> On 03/06/2014 04:12 PM, Valerie (Yu-Ching) Peng wrote:
>>
>> Last time when I was debugging/observing the session pools, it's not
>> just one or two sessions in the queue. It's at least hundreds possibly
>> thousands when I ran one of the SSL stress tests.
>>
>> 1) line 273, do you really mean to return when the
>> oldestSession.isLive() returns false??
>
> Yeah, got that backwards
>
>> 2) line 275 is changed to "return". Wouldn't this mean that the debug
>> statement on line 282-285 will rarely if ever be executed?
>
> replacing with a break should fix that
>
>> I think the main performance boost that we gained here is because we
>> switched from a synchronize everything approach to a very
>> fine-grained-synchronization one. The concern that I have is that is
>> whether we need additional synchronization will be required as there are
>> several calls involving the "pool" field in the Pool.release(Session)
>> method which have no additional synchronization involved... This seems a
>> bit too loose?
>
> I think it's fine, My checking if a session is old uses a peek, 
> leaving the entry in the queue.  Then if it is old and I want to 
> return it, pool.remove() is thread safe, so it returns false if the 
> entry is gone and I can exit the loop because the queue has changed 
> significantly and the closing of the session is skipped. Only the 
> thread that removed the session from the queue can close it.
>
> It does get complicated because the original version kept at least one 
> session in the queue, i tried to keep that in this version.  Ideally I 
> would have run through the queue closing all old session even if that 
> closed all op sessions; nevertheless, the above the peek keeps us from 
> running into a problem.
>
> I've run this on a stress test with 256 threads for 10 minutes using 
> AES and didn't have any exceptions or errors.
>
> I updated the webrev and with the change to use ConcurrentLinkedDeque 
> instead.
> http://cr.openjdk.java.net/~ascarpino/7107611/webrev.03/
>
>>
>> Regards,
>> Valerie
>>
>> On 03/06/14 12:59, Anthony Scarpino wrote:
>>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the security-dev mailing list