RFR: 8098581 SecureRandom.nextBytes() hurts performance with small size requests

[Anthony Scarpino]

On 12/03/2015 11:10 AM, Mike StJohns wrote:
> The more I look at this the less I'm comfortable with the change to
> non-synchronized.
>
> In general, there shouldn't be a need for a large amount of random data,
> especially not between multiple threads, so I'm not too sure the
> performance argument holds.

End to End testing on Solaris using WebLogic showed up ~20% faster page 
response times with this fix.  Profiling using JFR showed nextBytes() is 
a hot lock as the threads queued up wait to get random data.  A 
microbenchmark showed 3x better performance after the change.

It makes sense to have locking in the provider to protect against 
duplicate random data.  Synchronizing at the public API level of 
nextBytes(), is like having a synchronized Cipher.init(), it is way to 
high and impractical.

> What I do worry about is a provider that's not particularly smart about
> how it does things offering up data and not updating internal pointers
 > in a synchronized manner resulting in the possibility of the same
 > "random" data being served to multiple requestors.

I had that about that concern too.

There is no mention of a threaded guarantee by nextBytes() or 
SecureRandom in the documentation and I don't believe 'synchronized' is 
part of the public API that we are held to.

If we don't do this change because of the possibility someone's poor 
design, the only remedy I see is deprecating nextBytes() and replacing 
it with a new method.  To me, while that may be the safest move, that 
seems like a drastic step for the possibility of a poorly design.

> I think placing the
> synchronization at the glue class may be a better choice than depending
> on each and every provider doing the right thing.

I don't see how inserting a new class into this resolves the problem? 
The lock encompasses the whole getting of random numbers.

Tony

>
> Just a thought - Mike
>
>
>
> On 12/3/2015 1:38 PM, Seán Coffey wrote:
>> Hey Tony,
>>
>> looks like a fix that could come back to jdk8u-dev at some stage.
>> Thanks for taking this one on.
>>
>> Adding SecureRandom to the sunpkcs11-solaris.cfg file is a nice move.
>> Would you agree that JDK-8058455 could be closed out as a duplicate ?
>>
>> Regards,
>> Sean.
>>
>> On 01/12/2015 23:44, Anthony Scarpino wrote:
>>> Hi all,
>>>
>>> I'd like a review of this change.  It improves nextBytes()
>>> performance by allowing the random buffer to grow and shrink as
>>> random data is needed and remove the high level synchronization. Also
>>> disable SecureRandom for Solaris PKCS11 as it's not as fast as native.
>>>
>>> http://cr.openjdk.java.net/~ascarpino/8098581/webrev/
>>>
>>> Thanks
>>>
>>> Tony
>>
>>
>