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