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
