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