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

[Xue-Lei Fan]

Looks fine to me.

Thanks,
Xuelei

On 12/2/2018 3:48 AM, Weijun Wang wrote:
> Thanks for your support. Here is the updated webrev:
> 
>     https://cr.openjdk.java.net/~weijun/8210476/webrev.02/
> 
> I've made ctxt transient and added a readObject method, and a new test for this.
> 
> The cleaner action is now implemented as a named inner class.
> 
> Thanks
> Max
> 
>> On Dec 1, 2018, at 2:03 AM, Xue-Lei Fan <xuelei.fan at oracle.com> wrote:
>>
>> 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
>>>>>>>
>>>>>>
>