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
