Request for review, 6766644: Redefinition of compiled method fails with assertion "Can not load classes with the Compiler thread"

[Tom Rodriguez]

On Jan 26, 2011, at 1:39 PM, Daniel D. Daugherty wrote:

> Sorry for being late to this thread...
> 
> 
> On 1/25/2011 12:00 PM, Tom Rodriguez wrote:
>> On Jan 25, 2011, at 8:49 AM, Keith McGuigan wrote:
>> 
>>   
>> 
>>> Hello,
>>> 
>>> This code modifies the way that JVMTI compiled-method-load events are posted.  Previously, they were posted directly from the compiler thread, which could cause issues if the JVMTI event handling code made calls to RedefineClasses, since the compiler thread is unable to load classes if that is required.  This solution is to defer the posting of the events to the Service thread (formerly: LowMemoryDetector 
>>> thread) which is a Java thread and is able to load classes.
>>>     
>>> 
>> 
>> The posting appears to be asynchronous which I don't think will work.  The method could be unloaded or invalidated and freed before the event has been posted.  It has to be done synchronously I think.
>>   
>> 
> 
> Synchronous behavior does appear to be required for CompiledMethodLoad
> or at least the compiled method can't be unloaded until after this
> event is posted and the event handler returns.
> 
> CompiledMethodUnload has some restrictions about the memory not being
> reused for a newly generated compiled method before the unload event
> is sent.

Hmm.  That doesn't seem to be enforced anywhere.  I think the pending_compiled_method_unload events would have to lock the nmethod so it isn't freed but they aren't currently doing that.  I think keith is going to have to do that as well as part of the rewrite he's doing so that will fix that issue.

> On a good note, the unload event can be posted after the class
> is unloaded and there are suitable warnings about limited use of the
> 'method' and 'code_addr' fields.
> 
> It also looks like there must be order between the load and unload events:
> 
>     CompiledMethodLoad for foo
>     CompiledMethodUnload for foo
>     CompiledMethodLoad for foo (again)

Do you mean we can't have multiple versions of compiled code for the same method?  I don't think that's true or should be required.  nmethod freeing is very lazy and there's no guarantee that we will have completed unloading of an nmethod before we've created a new variation of it.  It would also be completely ok for a JVM to have multiple versions of the compiled code for a method.  Obviously the load and unload for a particular nmethod must be properly ordered.

> 
> which is going to mean coordination between the mechanisms for posting
> of both CompiledMethodLoad and CompiledMethodUnload events.
> 
>>  
>> I have a vague memory of someone saying that there were restrictions on what a JVMTI client was allowed to do in response to a CompiledMethodLoad but I can't find any evidence of that.  So is that just a bogus memory or is there some restriction like this?
>>   
>> 
> 
> Keith and I crawled over the current JVM/TI spec:
> 
>     http://download.oracle.com/javase/6/docs/platform/jvmti/jvmti.html
> 
> and we were unable to find anything that restricted being able to
> call the RedefineClasses() API from the CompiledMethodLoad event
> handler. I'm not saying your memory is bogus and there some things
> that we didn't catch until our 4th or 5th readings, but...

It seems pretty clear that there's aren't any specified restrictions so I think we just need to make it work right.

tom

> 
> Dan
> 
> 
> 
>>  
>> tom
>> 
>>   
>> 
>>> The "Service thread" now handles both low-memory detection and JVMTI deferred event posting.   I left the door open for other types of events to be deferred and posted via this mechanism in case we find other situations where posting events from a non-Java thread causes problems.
>>> 
>>> webrev:  
>>> http://cr.openjdk.java.net/~kamg/6766644/webrev.01/
>>> 
>>> 
>>> --
>>> - Keith
>>>     
>>> 
>> 
>>   
>>