Re: Review request for 6810254

[Mandy Chung]

Thanks Alan.  This is a good approach that avoids using reflection and 
addresses David's concern.  I was a bit aggressive to lazily instantiate 
Java*Access objects as many as I can.  I'll revise the fix and send out 
a new webrev.


Thanks
Mandy

Alan Bateman wrote:

Mandy Chung wrote:

6810254: Lazily instantiate the shared secret access objects

Webrev at:
  http://cr.openjdk.java.net/~mchung/6810254/webrev.00/

sun.misc.Java*Access objects are created at initialization time.  
However, they are not always needed.  They can be instantiated lazily 
when needed.  The fix is to add a static setSharedSecret() method to 
be called by sun.misc.SharedSecrets via reflection when the shared 
secret access object is requested.


Thanks
Mandy
It's good to see the setup of the shutdown hooks being removed from 
the initialization. However, I think it might be cleaner have each 
register itself lazily rather than SharedSecrets knowing about it. 
That has the added benefit that only the needed hooks are registered. 
It also avoids needing the reflection code. A possible downside is 
that each hook needs to know its place in the world. Attached is a 
(completely unpolished) patch that does this and perhaps it would be 
useful to try.


-Alan.

Re: Review request for 6810254

[David Holmes - Sun Microsystems]

Hi Mandy,

Note that my main concern is the use of reflection. If the class 
involved has not previously had it's reflection objects initialized then 
getDeclaredMethod can lead to a lot of native and Java-level allocation 
(I'm assuming this hasn't changed a great deal from the Java 5 code).


Even if there is memory to be allocated this might induce some GC churn 
in a new and unexpected place.


Maybe the lazy-initialization holder class pattern can be used instead eg:

 static {
  // setup access to this package in SharedSecrets
  sun.misc.SharedSecrets.setJavaNioAccess(...
 }

becomes:

 static class SharedSecretsHelper {
static {
  // setup access to this package in SharedSecrets
  sun.misc.SharedSecrets.setJavaNioAccess(...
   }
 }

and setSharedSecret(Class? cls) does:

  Class.forName(cls.getName() + $SharedSecretsHelper);

???

Leave it with you ...

Cheers,
David

Mandy Chung said the following on 03/06/09 07:18:

On 03/05/09 04:18, David Holmes - Sun Microsystems wrote:

Hi Mandy,

Isn't this kind of change risky? With static initialization you know 
that once the VM gets up and running then everything is in place. But 
with lazy-initialization (and using reflection no less!) there's a 
danger that when you try to initialize you're more likely to fail due 
to lack of memory or resources.  


That's a good point. I change the getSystemShutdownHooks() method to 
return a preallocated array.  The initialization of the hooks themselves 
 are not changed by this fix.  However, if the application shutdown hook 
adds the first file to be deleted on exit, the lazy initialization may 
cause some trouble.  I'll look into it and send out a new webrev.


I ran the jtreg tests and I am going to run the JCK tests to make sure 
no regression.


I can't quite tell exactly when these setSharedSecret methods will be 
called.


When SharedSecrets.getJava*Access() method is called, it will 
SharedSecrets.setSharedSecret() which in turn calls 
cls.setSharedSecret() method of the given cls.


BTW I think the comments copied into 
src/share/classes/java/io/DeleteOnExitHook.java need to be reviewed - 
they made sense when the code was java.io.File, but not now :)


Ok.

Thanks
Mandy


Cheers,
David

Mandy Chung said the following on 03/05/09 17:01:

6810254: Lazily instantiate the shared secret access objects

Webrev at:
  http://cr.openjdk.java.net/~mchung/6810254/webrev.00/

sun.misc.Java*Access objects are created at initialization time.  
However, they are not always needed.  They can be instantiated lazily 
when needed.  The fix is to add a static setSharedSecret() method to 
be called by sun.misc.SharedSecrets via reflection when the shared 
secret access object is requested.


Thanks
Mandy