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

[Vladimir Kozlov]

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);

  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().

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

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


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

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

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

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

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

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;
+  }

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

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

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

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

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

constantPoolOop.cpp:
  Why not use guarantee() for bad operants?
  Why you need separate scopes in resolve_bootstrap_specifier_at_impl()?

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

bytecodeInfo.cpp:
Don't add spaces into conditions, looks strange.
Remove commented code for inline ForceInline methods.

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

graphKit.cpp:
Remove commented debug print.
insert_argument() and remove_argument() are not used.


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/
>