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

[Roger Riggs]

Hi,

The Cleaner approach should be reasonable and we'd like to avoid
introducing new uses of finalize that will need to be removed later.
Tracking it to see if there are any issues is a good idea.

Thanks, Roger


On 11/30/2018 01:03 PM, Xue-Lei Fan 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
>>>>>>
>>>>>
>>