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

[Ivan Gerasimov]

Hi Weijun!

A few comments.

1)

Since PRNG is Serializable, shouldn't `ctxt` be declared transient?


2)

The documentation of Cleaner [1] suggests not to use lambdas for 
cleaning actions, as it may accidentally capture a reference to the 
object  being cleaned.

[1] 
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/ref/Cleaner.html 



3)

Do you know if it is true that the context handle might be used 
concurrently?

I couldn't find in the MSDN online documentation specific promises 
(there is a link to "Threading Issues with Cryptographic Service 
Providers" article, but the page is empty).

Would it make sense to maintain the only context handler in a static 
WeakReference<Context> field?

So that it is reused with all instances of PRNG and is cleared when the 
last instance of PRNG is gone.

Not sure if it is really necessary, as a normal application wouldn't 
probably need several instances of PRNG, but could prevent 
out-of-available-handles trouble in some extreme cases.


With kind regards,

Ivan.


On 11/26/18 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
>>>
>

-- 
With kind regards,
Ivan Gerasimov