Request for reviews (L): 6893081: method handle & invokedynamic code needs additional cleanup (post 6815692, 6858164)

[John Rose]

On Dec 2, 2009, at 10:20 AM, Christian Thalinger wrote:

> On Wed, 2009-12-02 at 10:10 -0800, Tom Rodriguez wrote:
>> what's the deal with the is_not_initialized changes in
>> linkResolver.cpp?  There's nothing inherently wrong with compiling
>> methods of a class which has not been initialized so what's changed?
>> I could understand checking !is_linked() though.
> 
> I think John has to jump in here...  -- Christian

OK, <jump/>.

On Dec 2, 2009, at 10:10 AM, Tom Rodriguez wrote:

> what's the deal with the is_not_initialized changes in linkResolver.cpp?  There's nothing inherently wrong with compiling methods of a class which has not been initialized so what's changed?  I could understand checking !is_linked() though.
> 
> Otherwise it looks good.

Here's the error I get when I disable that logic and run a simple -Xcomp test of JSR 292 stuff:

Internal Error at compileBroker.cpp:946, pid=74309, tid=2694092032 
Error: assert(!instanceKlass::cast(method->method_holder())->is_not_initialized(),"method holder must be initialized")

Here's the C stack as far as GDB goes:
#5  0x00013d06 in report_assertion_failure
#6  0x00354485 in CompileBroker::compile_method
#7  0x006a42af in CallInfo::set_common
#8  0x006a44d1 in CallInfo::set_static
#9  0x006a7002 in LinkResolver::resolve_static_call
#10 0x0071f7f0 in MethodHandles::resolve_MemberName
#11 0x00726fa9 in MHI_resolve_Mem
#12 (java stuff)

Here's the next part of the Java stack displayed by "call ps()":
 1 - sun.dyn.MethodHandleNatives.resolve(Native Method)
 2 - sun.dyn.MemberName$Factory.resolveInPlace(MemberName.java:497)
 3 - sun.dyn.MemberName$Factory.resolveOrNull(MemberName.java:509)
 4 - sun.dyn.MemberName$Factory.resolveOrFail(MemberName.java:514)
 5 - java.dyn.MethodHandles$Lookup.findStatic(MethodHandles.java:234)
 6 - dvm_richards.<clinit>(dvm_richards.java:3256)

My test routine here is an invokedynamic-based "richards" benchmark for JavaScript, but it could be anything whose <clinit> calls into the JSR 292 runtime.

What I get from this is that the next call after the is_not_initialized check in linkResolver will got to compile_method, which will complain about the initialization.  For some reason, we never got here before; you are right Tom to ask "what's changed?"  But it is already the case that the compile broker will crash when asked to compile an uninitialized method.

The backtrace above is forming method handles in a <clinit> method, so it can store them in static finals to use later as constants.  The method handle formation in this case involves a symbolic lookup (resolve_MemberName).  This is calling into the LinkResolver, which in turn is feeling the need (under -Xcomp) to compile the referred-to method.  This is a questionable policy, but I hope I can be agnostic about it for now.

But this points out what's changed:  Previously, anybody who would be using the LinkResolver to refer to a method M would be doing so with the immediate intention of calling it.  The JVM's invariants imply that M's class is initialized at that point.  With method handles, the rules are relaxed a little bit.  The "MemberName" API is allowed to look at class metadata before classes are initialized. By the time the call to Lookup.findStatic returns successfully, it will have created a DirectMethodHandle, and the code in MethodHandles::init_DirectMethodHandle will have run the initialization barrier logic, triggering (in this case) initialization of some class.

This points out some questions:

- Should method handle creation trigger the initialization of the destination class, or should the trigger be deferred until (if ever) the MH is called?  Basically, an MH is a new kind of early linkage, but linkage in the JVM does not trigger initialization; the first call does.  So maybe the long is wrong here.  (Will file a bug and fix later!)

- Should the call to the compile broker be repeated just after the initialization barrier?  I'm thinking this might be overkill.  If the target method is not compiled, the interpreter entry will cause it to be compiled.  I'm not understanding the reason the compile broker is called from LinkResolver; it seems like a mere -Xcomp hack.

Let me know what you think...

-- John