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