review request (XXXL): 7023639: JSR 292 method handle invocation needs a fast path for compiled code

[Christian Thalinger]

On Jul 12, 2012, at 6:27 PM, Vladimir Kozlov wrote:

> John,
> 
> sharedRuntime_sparc.cpp:
> Why casting to (int)? Also use pointer_delta(code_end, code_start,1):
> +  __ set((int)(intptr_t)(code_end - code_start), temp2_reg);

Done.

> 
> You bound L_fail label twice, it should be local in range_check(). Use brx() instead of br() since you compare pointers. And use cmp_and_brx_short() if delayed instruction is nop().

Done.

> 
> Use fatal() instead of guarantee:
> guarantee(false, err_msg("special_dispatch=%d", special_dispatch));

Done.

> 
> interpreter_sparc.cpp:
> In generate_method_entry() use fatal() instead of ShouldNotReachHere():
> fatal(err_msg("unexpected method kind: %d", kind));

Good idea.  Done.

> 
> 
> methodHandles_sparc.cpp:
> In MethodHandles::verify_klass() calls restore() should be after BINDs.

Argh.  I haven't seen you've found this bug.  It took me a while to debug this :-)

> 
> In MethodHandles::jump_from_method_handle() use cmp_and_br_short(temp, 0, )

Done.

> 
> Instead of 100 use strlen(name)+50:
> +    char* qname = NEW_C_HEAP_ARRAY(char, 100);
> +    jio_snprintf(qname, 100,

Done.

> 
> sharedRuntime_x86_32.cpp:
> sharedRuntime_x86_64.cpp:
> The same problem with L_fail label as in sharedRuntime_sparc.cpp.

Done.

> 
> templateInterpreter_x86_32.cpp:
> templateInterpreter_x86_64.cpp:
> Again use use fatal() instead of ShouldNotReachHere() in generate_method_entry()

Done.

> 
> I see in several files next code pattern. Should we call throw_IncompatibleClassChangeError() as we do in other places?:
> 
> +  if (!EnableInvokeDynamic) {
> +    // rewriter does not generate this bytecode
> +    __ should_not_reach_here();
> +    return;
> +  }

Hmm.  This really should not happen and EnableInvokeDynamic is on by default anyway.  I doubt someone turns it off.

> 
> c1_FrameMap.cpp:
> Why is ShouldNotReachHere() for mh_invoke in  FrameMap::java_calling_convention()?

That was old, unused code.  Removed.

> 
> c1_GraphBuilder.cpp:
> add parenthesis:
> const bool is_invokedynamic = code == Bytecodes::_invokedynamic;

Done.

> 
> nmethod.cpp:
> Don't put printing nmethod's addresses under Verbose flag.

Leftover.  Removed.

> 
> linkResolver.cpp:
> Can you replace infinite for(;;) in resolve_invokedynamic() with finite loop since the body is executed once or twice.

Hmm.  What limit do you have in mind?

> 
> templateInterpreter.cpp:
> why you need additional {} around the loop?

We don't.  I think it was used for better visibility of the MH code.  Removed.

> 
> constantPoolOop.cpp:
> Why not use guarantee() for bad operants?

Not sure.  John?

> Why you need separate scopes in resolve_bootstrap_specifier_at_impl()?

To keep oops from being visible.  Might result in bad crashes.

> 
> symbol.cpp:
> The loop in index_of_at() should be for(; scan <= limit; scan++) and after loop return -1.

Done.

> 
> bytecodeInfo.cpp:
> Don't add spaces into conditions, looks strange.

It's more readable:

  if ( callee->is_native())                     return "native method";
  if ( callee->is_abstract())                   return "abstract method";
  if (!callee->can_be_compiled())               return "not compilable (disabled)";
  if (!callee->has_balanced_monitors())         return "not compilable (unbalanced monitors)";
  if ( callee->get_flow_analysis()->failing())  return "not compilable (flow analysis failed)";

vs.

  if (callee->is_native())                      return "native method";
  if (callee->is_abstract())                    return "abstract method";
  if (!callee->can_be_compiled())               return "not compilable (disabled)";
  if (!callee->has_balanced_monitors())         return "not compilable (unbalanced monitors)";
  if (callee->get_flow_analysis()->failing())   return "not compilable (flow analysis failed)";

You REALLY want me to remove it? ;-)

> Remove commented code for inline ForceInline methods.

Agreed.  We need to revisit that anyway.

> 
> callGenerator.cpp:
> Please, decide which code to use: +#if 1. And I don't think new code is correct.

PredictedDynamicCallGenerator is dead.  Removed.

> 
> graphKit.cpp:
> Remove commented debug print.

Done.

> insert_argument() and remove_argument() are not used.

Correct.  Removed.

I will refresh the review patch in mlvm.  Thank you!

-- Chris

> 
> 
> Vladimir
> 
> John Rose wrote:
>> As some of you have noticed, Chris Thalinger, Michael Haupt, and I have been working on the mlvm patches [1] for JEP-160 [2] for several months.  These changes make method handles more optimizable.  They refactor lots of "magic" out of the JVM and into more manageable Java code.
>> To get an idea of how much "magic" is being removed, consider that the change removes 12,000 lines of non-comment code from the JVM, including much assembly code.  It inserts 4900 lines of non-comment code.
>> These changes are now stable enough to integrate.  They pass jtreg tests in a number of execution modes and platforms.  They also correctly run various JRuby and Nashorn test programs.  Although there are no performance gains to boast about at present, these changes clear the ground for long-term optimization work.
>> Here is the webrev [3], for review and integration into JDK 8 via hotspot-comp/hotspot/.
>> Because of the large size of this change set, we request that reviewers would clearly designate which files they are reviewing.  That way we may be able to divide up the work a little more effectively.
>> Also, because of the scope of the change, we may respond to some comments by promising to address an issue in a future change set.  If necessary, we will file tracking bugs to make sure nothing gets dropped.  We have been working on this for months, and expect to make many further changes.
>> The immediate need to get the changes in is twofold:  First, some bugs (involving symbolic references off the boot class path) require the new Lambda Form intermediate representation, which is "off-BCP-clean".  Second, we need to commit our pervasive changes to the JVM sooner rather than later, so they can be properly integrated with other pervasive changes, such as metadata changes.
>> An associated webrev for hotspot-comp/jdk/ will be posted soon; it is already present on mlvm-dev for the curious to examine.  (This change set also deletes a lot of old code.)
>> Thanks in advance,
>> — John
>> [1] http://hg.openjdk.java.net/mlvm/mlvm/hotspot/file/tip/meth-lazy-7023639.patch
>> [2] http://openjdk.java.net/jeps/160
>> [3] http://cr.openjdk.java.net/~jrose/7023639/webrev.00/