RFR 8210476: sun/security/mscapi/PrngSlow.java fails with Still too slow

[Xue-Lei Fan]

Hi Weijun,

I think you made a significant improvement of the performance, with less 
timing and resources.  I don't think my concerns are strong enough to 
prevent this fix from moving forward.

I'm fine with your update.  You can move forward as it is, or using 
finalize() instead.

For my concerns, if you go with Cleaner, we can open a new RFE for the 
tracking if we have a good idea in the future.

Thanks,
Xuelei

On 11/30/2018 3:47 AM, Weijun Wang wrote:
> Hi Xuelei, and Ivan who replied in another mail,
> 
>> On Nov 29, 2018, at 4:31 AM, Xue-Lei Fan <xuelei.fan at oracle.com> wrote:
>>
>> Do you know, is there any other way except Cleaner and finalize() to clean up the allocated resources?
> 
> I don't know any other automatic mechanisms. There is AutoClosable but SecureRandom has not implemented it and we cannot expect people using try-with-resources on it.
> 
>>
>> I'm not very sure of the use of static Cleaner:
>> 1. a daemon thread will run underlying.
>> 2. the number of registered actions could be huge in some circumstances.
> 
> Anyway, I think any fix that reuses the context is to change a time performance issue into a resource performance issue because we cannot precisely control the resource cleanup.
> 
> I don't have enough data on how often people use Windows-PRNG, how many objects they create, how many nextBytes they call on a single object, and how they use it in multiple threads. So it's quite clueless to determine which solution is the best.
> 
> Both this bug and the previous one (JDK-6449335) are not reported by external customers. Therefore I prefer retargeting the bug to the next release and problem list the test at the moment. In the last 1700 mach5 jobs with this test, there were 4 timeouts.
> 
> Thanks
> Max
> 
> p.s. We might reimplement using CNG but CNG also has its own problem (no easy way to implement setSeed).
> 
>>
>> I'm not very sure if it could be a scalability bottleneck or not.
>>
>> Xuelei
>>
>>
>> On 11/26/2018 5:33 PM, Weijun Wang wrote:
>>> Ping again
>>>> On Nov 20, 2018, at 5:33 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>>
>>>> Webrev updated at
>>>>
>>>>    https://cr.openjdk.java.net/~weijun/8210476/webrev.01/
>>>>
>>>> The only change is that there is a single Cleaner now for the whole PRNG class. Otherwise, each will maintain its own thread.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>> On Nov 11, 2018, at 11:30 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>>>>
>>>>> Please take a review at
>>>>>
>>>>>   https://cr.openjdk.java.net/~weijun/8210476/webrev.00/
>>>>>
>>>>> Before this fix, every PRNG::nextBytes calls all of CryptAcquireContext, CryptGenRandom, and CryptReleaseContext. Now, CryptAcquireContext is called once in PRNG::new, and CryptReleaseContext is called by a Cleaner, and nextBytes only calls CryptGenRandom.
>>>>>
>>>>> I haven't read about thread-safety in any MS document, the current Windows-PRNG service is marked ThreadSafe=true (in SunMSCAPI.java). If we cannot be really sure, we can change it to false.
>>>>>
>>>>> I've downloaded nearly 1000 Mach5 runs of this test, the enhancement is so good that I adjusted the test to be stricter.
>>>>>
>>>>> 	Before	After
>>>>> 	------	-----
>>>>> Count	897	74
>>>>> Min	0.38	0.008
>>>>> Ave	0.97	0.011
>>>>> Max	5.81	0.021
>>>>>
>>>>> Please advise me if the following usage of Cleaner is correct because I really haven't observed the releaseContext method being called.
>>>>>
>>>>> +        Cleaner.create().register(this,
>>>>> +                () -> releaseContext(ctxt));
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>
>