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....@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....@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.w...@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.w...@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 >>>>>> >>>>>
