hg: mlvm/mlvm/jdk: meth-lazy: add stronger typing to field accessor base pointers; constructor bug fix
Changeset: c73fecb5e075 Author:jrose Date: 2012-07-17 17:03 -0700 URL: http://hg.openjdk.java.net/mlvm/mlvm/jdk/rev/c73fecb5e075 meth-lazy: add stronger typing to field accessor base pointers; constructor bug fix ! meth-lazy-7023639.bmh.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
hg: mlvm/mlvm/hotspot: meth-lazy: update for review comments
Changeset: 9717ee54c9f5 Author:twisti Date: 2012-07-17 17:00 -0700 URL: http://hg.openjdk.java.net/mlvm/mlvm/hotspot/rev/9717ee54c9f5 meth-lazy: update for review comments ! meth-lazy-7023639.review.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: Crash using sun.misc.Unsafe.static
On 07/18/2012 12:55 AM, John Rose wrote: > On Jul 16, 2012, at 4:06 PM, Krystal Mok wrote: > >> And you're right that this has to do with the PermGen removal >> project. The difference comes from [1], which is a part of CR 7017732. >> To be specific, before the 7017732, static fields are stored in the >> instanceKlass of a Java class; an instanceKlass (or its enclosing >> klassOopDesc to be exact) is not an Java object. >> After 7017732, static fields are moved to the tail of java.lang.Class >> instances, which are Java objects. >> >> So to answer your question, you just shouldn't expect the the code to >> work in JDK6/HotSpot. > > The API for unsafe access to statics is designed to allow > implementations to do one of two things: > > 1. store a static variable at a fixed offset within a managed object > (addressable via Java references) > 2. store a static variable at an arbitrary but fixed 64-bit VA (in > which case the object base is just null) > > The JDK 6 system is in a middle state, where the instanceKlass is a > managed object and can move is not compatible with Java references. > I think there was a time when non-Java oops could be manipulated via > Java reference variables, but the practice has always been rather… unsafe. > > This could be fixed in JDK 6, but it's probably not worth it. I > recommend spinning bytecoded accessors for statics on JDK 6. On JDK 7 > and later, the Class is the base (as Kris pointed out). This is > probably how it's going to stay. I spin bytecode by default if the field is visible from the callsite and wanted to use unsafe with a constant base object if the field is not visible. So to workaround that bug, I store the base object in a non-constant field exactly like reflection does, so it works on jdk6. Exactly, I should try to detect if the VM is hotspot or not because neither JRockit nor J9 have the same issue. > > Thanks for the report. > > — John > > P.S. For multi-tenancy VMs, the addressing arithmetic for statics > would need to take into account the task ID. But the above design > (baseOrNull + longOffset) still works, since the unsafe API doesn't > say how the two components get combined. Rémi ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: review request (XXXL): 7023639: JSR 292 method handle invocation needs a fast path for compiled code
>> 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())
Re: review request (XXXL): 7023639: JSR 292 method handle invocation needs a fast path for compiled code
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 execut
hg: mlvm/mlvm/jdk: meth-lazy: added reinvokerTarget method to BMH species
Changeset: 72d2a3526502 Author:twisti Date: 2012-07-17 16:02 -0700 URL: http://hg.openjdk.java.net/mlvm/mlvm/jdk/rev/72d2a3526502 meth-lazy: added reinvokerTarget method to BMH species ! meth-lazy-7023639.bmh.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: Crash using sun.misc.Unsafe.static
On Jul 16, 2012, at 4:06 PM, Krystal Mok wrote: > And you're right that this has to do with the PermGen removal project. The > difference comes from [1], which is a part of CR 7017732. > To be specific, before the 7017732, static fields are stored in the > instanceKlass of a Java class; an instanceKlass (or its enclosing > klassOopDesc to be exact) is not an Java object. > After 7017732, static fields are moved to the tail of java.lang.Class > instances, which are Java objects. > > So to answer your question, you just shouldn't expect the the code to work in > JDK6/HotSpot. The API for unsafe access to statics is designed to allow implementations to do one of two things: 1. store a static variable at a fixed offset within a managed object (addressable via Java references) 2. store a static variable at an arbitrary but fixed 64-bit VA (in which case the object base is just null) The JDK 6 system is in a middle state, where the instanceKlass is a managed object and can move is not compatible with Java references. I think there was a time when non-Java oops could be manipulated via Java reference variables, but the practice has always been rather… unsafe. This could be fixed in JDK 6, but it's probably not worth it. I recommend spinning bytecoded accessors for statics on JDK 6. On JDK 7 and later, the Class is the base (as Kris pointed out). This is probably how it's going to stay. Thanks for the report. — John P.S. For multi-tenancy VMs, the addressing arithmetic for statics would need to take into account the task ID. But the above design (baseOrNull + longOffset) still works, since the unsafe API doesn't say how the two components get combined.___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: Crash using sun.misc.Unsafe.static
On 07/17/2012 01:06 AM, Krystal Mok wrote: > Hi Remi, > > Looks like it's a trap if you're running this on JDK6's HotSpot VM. If > you're running a debug build of the same VM, you should hit an > assertion before hitting the ShouldNotReachHere() part. > > opto/type.cpp:2477 > assert(o->is_java_object(), "must be java language object"); > > Even though the source code of JDK6u33 is not open sourced, you could > try a similar version of HotSpot VM from OpenJDK, namely the > hsx/hsx20/baseline branch, or jdk6/jdk6/hotspot. Make a fastdebug > build and you should see the assertion. > > And you're right that this has to do with the PermGen removal project. > The difference comes from [1], which is a part of CR 7017732. > To be specific, before the 7017732, static fields are stored in the > instanceKlass of a Java class; an instanceKlass (or its enclosing > klassOopDesc to be exact) is not an Java object. > After 7017732, static fields are moved to the tail of java.lang.Class > instances, which are Java objects. > > So to answer your question, you just shouldn't expect the the code to > work in JDK6/HotSpot. Hi Krys, Too bad because I wanted to use a similar code in the jsr292 backport (a Java agent that enable dynamic language runtime written with invokedynamic to run on a jdk6 stock VM) This remember me another question, the java agent use java.lang.instrument.Instrumentation.retransform to change code at runtime and it seems that using retransform disable OSR on the retransformed class. Is there a reason for that ? > > [1]: > http://hg.openjdk.java.net/jdk7/hotspot-gc/hotspot/diff/c7f3d0b4570f/src/share/vm/prims/unsafe.cpp Rémi ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
hg: mlvm/mlvm/jdk: meth: make BMHs be more strongly typed; various cleanups
Changeset: f07d59dc449e Author:jrose Date: 2012-07-17 05:25 -0700 URL: http://hg.openjdk.java.net/mlvm/mlvm/jdk/rev/f07d59dc449e meth: make BMHs be more strongly typed; various cleanups ! meth-lazy-7023639.bmh.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
hg: mlvm/mlvm/hotspot: meth: remove a bunch more dead code (CMH, MTF)
Changeset: ad665697620c Author:jrose Date: 2012-07-17 05:23 -0700 URL: http://hg.openjdk.java.net/mlvm/mlvm/hotspot/rev/ad665697620c meth: remove a bunch more dead code (CMH, MTF) ! meth-lazy-7023639.review.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
Re: failure compiling coroutine.cpp when coro guard enabled
I just pushed a fix for this problem... - Lukas Am 2012-07-16 07:40, schrieb John Rose: On Jul 15, 2012, at 3:56 PM, Stephen Bannasch wrote: I'm getting this error compiling coroutine.cpp when I add "coro" to my guards coroutine.cpp:111: error: 'startInternal_method_name' is not a member of 'vmSymbols' The problem is that the coro.patch file did not apply cleanly. A recent rebasing to hotspot-comp has probably broken the coro script. A relevant interfering change is this, in hotspot-comp: changeset: 3281:0105f367a14c user:rbackman date:Tue Mar 06 12:36:59 2012 +0100 summary: 7160570: Intrinsification support for tracing framework The script you use to apply the patches will have reported an error, something like this: 1 out of 5 hunks FAILED -- saving rejects to file src/share/vm/classfile/vmSymbols.hpp.rej If the patches fail to apply, there is little or no hope that the build process will work. --- John ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
hg: mlvm/mlvm/hotspot: coro: fix coro.patch problem with vmSymbols.hpp
Changeset: c489641155e3 Author:Lukas Stadler Date: 2012-07-17 11:59 +0200 URL: http://hg.openjdk.java.net/mlvm/mlvm/hotspot/rev/c489641155e3 coro: fix coro.patch problem with vmSymbols.hpp ! coro.patch ___ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
