review (L) for 7017732: move static fields into Class to prepare for perm gen removal

[Vladimir Kozlov]

Looks good.

Vladimir

Tom Rodriguez wrote:
> On Feb 9, 2011, at 5:51 PM, Vladimir Kozlov wrote:
> 
>> Tom Rodriguez wrote:
>>> I removed the no longer correct part.  Did you want something added?
>> Is next description is still valid?
>>
>>  77 //    [EMBEDDED Java vtable             ] size in words = vtable_len
>>  78 //    [EMBEDDED static oop fields       ] size in words = static_oop_fields_size
>>  79 //    [         static non-oop fields   ] size in words = static_field_size - static_oop_fields_size
>>  80 //    [EMBEDDED nonstatic oop-map blocks] size in words = nonstatic_oop_map_size
> 
> I removed lines 78 and 79 but the rest is still correct I think.
> 
>>>> I don't understand changes in Unsafe_StaticFieldBaseFromClass(),
>>>> it should return handle but you just return original jobject.
>>> We're supposed to return the object to use as the base of unsafe static field references.  That's just the java.lang.Class instance which was passed in so we simply return it.  What would you expect it to do?  I guess we might want to unhandle it and return a new local one in case the original one wasn't a local handle.  Is that what you're getting at?
>> Thank you for explaining. Code is good.
> 
> I've changed it to return a new local handle as that seems less surprising.
> 
> @@ -704,7 +704,7 @@ UNSAFE_ENTRY(jobject, Unsafe_StaticField
>    if (clazz == NULL) {                                                                                                                               
>      THROW_0(vmSymbols::java_lang_NullPointerException());                                                                                           
>    }                                                                                                                                                 
> -  return JNIHandles::make_local(env, java_lang_Class::as_klassOop(JNIHandles::resolve_non_null(clazz)));                                             
> +  return JNIHandles::make_local(env, JNIHandles::resolve_non_null(clazz));                                                                           
>  UNSAFE_END                                                                                                                                           
>                                                                                                                                                      
>  UNSAFE_ENTRY(void, Unsafe_EnsureClassInitialized(JNIEnv *env, jobject unsafe, jobject clazz))
> 
> I've regenerated the webrev.
> 
> tom
> 
>> Vladimir
>>
>>> tom
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Tom Rodriguez wrote:
>>>>> I've updated the webrev with this change, fixed a couple macro names that still referenced InstanceKlass and fixed the alignment of the continuation slashes.
>>>>> tom
>>>>> On Feb 9, 2011, at 3:52 AM, Christian Thalinger wrote:
>>>>>> On Feb 8, 2011, at 11:01 PM, Tom Rodriguez wrote:
>>>>>>> http://cr.openjdk.java.net/~never/7017732
>>>>>> src/cpu/x86/vm/c1_CodeStubs_x86.cpp:
>>>>>>
>>>>>> 315     Register tmp = rax;
>>>>>> 316     Register tmp2 = rbx;
>>>>>> 317     if (_obj == tmp) tmp = rcx;
>>>>>> 318     else if (_obj == tmp2) tmp = rcx;
>>>>>> 319     __ push(tmp);
>>>>>> 320     __ push(tmp2);
>>>>>> 321     __ get_thread(tmp);
>>>>>> 322     __ movptr(tmp2, Address(_obj, java_lang_Class::klass_offset_in_bytes()));
>>>>>> 323     __ cmpptr(tmp, Address(tmp2, instanceKlass::init_thread_offset_in_bytes() + sizeof(klassOopDesc)));
>>>>>> 324     __ pop(tmp2);
>>>>>> 325     __ pop(tmp);
>>>>>>
>>>>>> Why do you change tmp if _obj == tmp2?  I think line 318 isn't required at all since if _obj == tmp2 it gets saved and restored anyway.  What about this:
>>>>>>
>>>>>> 315     Register tmp = rax;
>>>>>> 316     Register tmp2 = rbx;
>>>>>> 319     __ push(tmp);
>>>>>> 320     __ push(tmp2);
>>>>>> 322     __ movptr(tmp2, Address(_obj, java_lang_Class::klass_offset_in_bytes()));
>>>>>> 321     __ get_thread(tmp);
>>>>>> 323     __ cmpptr(tmp, Address(tmp2, instanceKlass::init_thread_offset_in_bytes() + sizeof(klassOopDesc)));
>>>>>> 324     __ pop(tmp2);
>>>>>> 325     __ pop(tmp);
>>>>>>
>>>>>> -- Christian
>