Request for review (S): 7174928: JSR 292: unresolved invokedynamic call sites deopt and osr infinitely

[Christian Thalinger]

On Jun 12, 2012, at 11:21 AM, Vladimir Kozlov wrote:

> Looks good to me. Only concern is how set_exception_handlers() affects new code.

Good point.  I looked at this again and the only other code using this (and where I got this pattern from in the first place) is NewMultiArray.  And there it's used correctly:

  CodeEmitInfo* patching_info = NULL;
  if (!x->klass()->is_loaded() || PatchALot) {
    patching_info = state_for(x, x->state_before());

    // cannot re-use same xhandlers for multiple CodeEmitInfos, so
    // clone all handlers.  This is handled transparently in other
    // places by the CodeEmitInfo cloning logic but is handled
    // specially here because a stub isn't being used.
    x->set_exception_handlers(new XHandlers(x->exception_handlers()));
  }
  CodeEmitInfo* info = state_for(x, x->state());

While the code for Invoke does this:

  CodeEmitInfo* info = state_for(x, x->state());

  // invokedynamics can deoptimize.
  CodeEmitInfo* deopt_info = x->is_invokedynamic() ? state_for(x, x->state_before()) : NULL;

<...skip a lot of code...>

        x->set_exception_handlers(new XHandlers(x->exception_handlers()));

In fact the cloned exception handlers are not used but rather the original ones are used twice.  Currently I'm running some tests and we emit invokedynamics with exception handlers so this code is exercised.  I never saw a bug related to this so it might not be required but I'm not an expert here.

-- Chris

> 
> Vladimir
> 
> Christian Thalinger wrote:
>> http://cr.openjdk.java.net/~twisti/7174928
>> 7174928: JSR 292: unresolved invokedynamic call sites deopt and osr infinitely
>> Reviewed-by:
>> While testing a new flag I want to introduce (TraceOSR) I noticed that
>> unresolved invokedynamic call sites compiled in C1 deoptimize and OSR
>> infinitely.  The reason for that is we deoptimize but we don't have machinery
>> in C1 to tell to throw the method out and recompile it.  So we just enter the
>> same method again and hit the same unresolved call site.
>> The fix is to conditionally jump to the deopt stub in generated code if the
>> CallSite object is null.  Handling it this way it is not required to
>> recompile the code.
>> Tested with JRuby's bench_red_black.rb
>> src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
>> src/share/vm/c1/c1_LIRGenerator.cpp
>> src/share/vm/interpreter/interpreterRuntime.cpp
>> src/share/vm/oops/methodOop.cpp
>> src/share/vm/oops/methodOop.hpp