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/
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
