On Jul 17, 2012, at 4:32 PM, Vladimir Kozlov wrote:
> >> 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?
>
> It is not loop by logic. The code is trying to avoid double the check for
> CallSite has been bound already. I think it could be re-arranged as next:
>
> + Handle bootstrap_specifier;
> + // Check if CallSite has been bound already:
> + ConstantPoolCacheEntry* cpce = pool->cache()->secondary_entry_at(index);
> + if (cpce->is_f1_null()) {
> + int pool_index =
> pool->cache()->main_entry_at(index)->constant_pool_index();
> + oop bsm_info = pool->resolve_bootstrap_specifier_at(pool_index, CHECK);
> + assert(bsm_info != NULL, "");
> + // FIXME: Cache this once per BootstrapMethods entry, not once per
> CONSTANT_InvokeDynamic.
> + bootstrap_specifier = Handle(THREAD, bsm_info);
> + }
> + if (!cpce->is_f1_null()) {
> + methodHandle method(THREAD, cpce->f2_as_vfinal_method());
> + Handle appendix(THREAD, cpce->has_appendix() ? cpce->f1_appendix() :
> (oop)NULL);
> + result.set_handle(method, appendix, CHECK);
> + return;
> + }
Much better. Done.
-- Chris
>
> >> bytecodeInfo.cpp:
> >> Don't add spaces into conditions, looks strange.
> > You REALLY want me to remove it? ;-)
>
> No, you can leave it as it is.
>
> Vladimir
>
> Christian Thalinger wrote:
>> 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
[email protected]
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
