review (S) for 6656830: assert((*p)->is_oop(), "expected an oop while scanning weak refs")

[Vladimir Kozlov]

Good.

Vladimir

On 6/18/10 5:01 PM, Tom Rodriguez wrote:
>
> On Jun 18, 2010, at 2:32 PM, Vladimir Kozlov wrote:
>
>> Tom,
>>
>> I think _jmethod_id could be NULL when unload request happened between publishing nmethod and call to post_compiled_method_load_event() since it is called outside lock so post_compiled_method_load_event() is not called yet.
>> And, for the same reason, call method()->jmethod_id() in post_compiled_method_load_event() can have the same original problem you are fixing. Do I make sense or it is rubbish?
>
> The nmethod can't be unloaded at this point because there are strong references to the method being compiled from the compile broker, the CI and the methodHandle in the register_method.
>
>> We can avoid it if we call get_and_cache_jmethod_id() in nmethod::new_nmethod() and nmethod::new_native_nmethod().
>
> I wanted to avoid creating the jmethodID unless JVMTI was active since the semantics of jmethodIDs require that they are never freed, so it would be a leak all the time.
>
>> Also mh is not used anymore in jvmtiCodeBlobEvents.cpp
>> 326   methodHandle mh(nm->method());
>
> I've deleted that.
>
> tom
>
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> While investigating the sweeper bug I ended up diagnosing this one too.  I already have one review from Dan since he helped me work it out.
>>> http://cr.openjdk.java.net/~never/6656830
>>> 6656830: assert((*p)->is_oop(),"expected an oop while scanning weak refs")
>>> Reviewed-by: dcubed
>>> This is a long standing bug with the handling of jmethodIDs by the
>>> logic for notifying of code unloading.  The compiled method unload
>>> event passes the jmethodID as part of the event so you need to look it
>>> up when an nmethod is unloaded because the class it's associated with
>>> is being unloaded.  jmethodIDs are loosely coupled to the methodOop so
>>> there's an array of them in the instanceKlass and they are searched
>>> when the jmethodID for a methodOop is required.  jmethodIDs are really
>>> just weak JNI references so when their class is being unloaded they
>>> all become null so it's impossible to look up the right jmethodID for
>>> an nmethod which is being unloaded.  In the failing test this results
>>> in a new jmethodID being allocated in the middle of the GC which can
>>> later lead to crashes since root set has been extended so stale oops
>>> are being processed in later phases of the GC.  The simplest fix is to
>>> cache the jmethodID in the nmethod so that it can be used in the
>>> unload notification.
>>> I also added logic to complain if new weak references are created
>>> while a GC is active.
>