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