Re: [cp-patches] [generics] RFC : implementation of jjava.lang.instrument package

[Nicolas Geoffray]

Hi Mark

Mark Wielaard wrote:

> Hi Nicolas,
>
> On Mon, 2005-11-28 at 19:08 +0100, Nicolas Geoffray wrote:
>  
>
> > Here's two files, InstrumentationImpl and VMInstrumentationImpl.
> > I put all native vm specific methods in VMInstrumentationImpl, but
> > if you think it's not necessary, it's ok with me to have all these methods
> > in InstrumentationImpl.
> >    
>
> This looks pretty good. Thanks!
> Some comments and questions:
>
> InstrumentationImpl should implement Instrumentation (then a couple of
> methods need to be made public).
>
>  
Oops, so obvious that I forgot it :)

> I believe the Vector of transformaers is handled correctly even when
> manipulated by multiple threads. But I like using an ArrayList and
> Iterator and do explicit synchronization (but in this case that is
> clearly just a personal preference, so feel free to ignore if you
> disagree that it is clearer.)
>
>  
OK for an ArrayList, it's just that I'm not really used to generics
and Vector seemed easier

> How atomic do we want applyRedefineClasses() to be? Instead of looping
> through all classes to redefine and calling applyRedefineClass() on each
> one, it might be a good idea to have an applyRedefineClasses(Object[])
> in VMInstrumentationImpl to redefine them all at the same time after
> preparing them all. 
You're right. It's better to apply them all.

> Also if something went wrong with the transformation
> of any of them we might want to have a cancelRedefineClass() to let the
> VM know we won't be applying any prepared classes.
>
>  
I had in mind that the VM wouldn't need a cancel method. The
prepareRedefineClass gives a vm internal representation
of a class, and if redefinition is canceled this representation would
be garbage collected and forgotten by the vm. But maybe some vm
would like to be noticed. I added the method since it might be usefull.

> Why does the private callTransformers() implementation take a
> ProtectionDomain? Isn't that classBeingRedefined.getProtectionDomain()?
>
>  
Yes, but the classBeingRedefined might be null, if the method is called
by the vm from the native ClassLoader.defineClass method. Therefore,
the vm gives as arguments the protectionDomain it has.

> About the calltransformers FIXME. What else can/should we do here?
>
>  
I have no idea  for now : the description of Sun doesn't specify which 
behaviour
sould be adopted. At first I wanted to skip the transformer that raises 
the exception.
But then, it might be the transformer called before that returned an 
illegal class.
And so on. We can know if the byte[] given is correct by forcing the vm 
to read it, but
that would not be efficient. I chose to return the initial byte[], 
because I didn't want to make
assumptions on how to treat transformers. I should do some tests with 
the sun implementation :)

> Normally we like to provide some default/noop implementation for the VM
> reference classes. Is it possible to provide the necessary hooks in
> ClassLoader/VMClassLoader to call the transformers just before
> defineClass() is called? 
It's possible, but it could break all existing vms lying on the vm 
reference classes.
The VMClassLoader would need an extra defineClass method and an extra field
for an InstrumentationImpl object. The extra defineClass method would 
check if the
InstrumentationImpl is null and if not, call the 
InstrumentationImpl.callTransformers
method.

The extra field can only be set by the command parsing method, because 
it's created
when an instrument agent is created.

> If we do this wouldn't it be slightly easier to
> have all these classes in vm/reference/java/lang to make package access
> easier?
>
>  
Probably. I have set private the callTransformers method, but it could 
be set protected.

> An example how this is all supposed to work together when a VM
> implements this VM interface would be nice.
I could propose something with my vm (jnjvm). But I have to make sure 
the generics branch works
with it :)

> Also to make clear whether
> the premain() method is an instance of static method, what command line
> extensions/flags are expected, etc.
>
>  
Yeah, documentation to write! :). I will create a documentation file for 
the java.lang.instrument package,
and change the vm integration guide (should I create a new paragraph for 
generics?)

Cheers,
Nicolas