review (L) for 7017732: move static fields into Class to prepare for perm gen removal
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Mar 16 15:45:14 PDT 2011
Tom,
It would be nice if you create a separate bug for this issue.
Is http://cr.openjdk.java.net/~never/7017732/ webrev is the latest?
Changes in few files are empty:
src/share/vm/memory/referenceProcessor.cpp
src/share/vm/utilities/globalDefinitions.hpp
src/share/vm/memory/space.cpp
universe.cpp: could you move next code into a separate method
(I mean, for example, set_offset_of_Class_static_fields())?:
+ // Cache the start of the static fields
+
instanceMirrorKlass::set_offset_of_static_fields(instanceMirrorKlass::cast(SystemDictionary::Class_klass())->size_helper()
<< LogHeapWordSize);
compile.cpp: Could you put parentheses around expressions in conditions code?:
+ if ((tinst->const_oop() != NULL) &&
+ (tinst->klass() == ciEnv::current()->Class_klass()) &&
+ (tinst->offset() >=
(tinst->klass()->as_instance_klass()->size_helper() * wordSize))) {
and in the following assert.
The rest is looks good.
Vladimir
Tom Rodriguez wrote:
> Hopefully this is the final version of this webrev. I found and fixed a preexisting bug with ScavengeRootsInCode that boiled down to a race between updating the oops in the nmethod and rewriting the oops in the instruction stream. The fix is to call fix_oop_relocations after all the nmethods have been visited by the scavenger. That changes nmethod.cpp and codeCache.cpp which you can see in the updated version of the webrev. I plan on pushing this to hotspot-gc once the weekly sync and promotion has been done. Thanks for all the reviews.
>
> tom
>
> On Mar 8, 2011, at 8:23 PM, Tom Rodriguez wrote:
>
>> On Mar 8, 2011, at 4:59 PM, Vladimir Kozlov wrote:
>>
>>> Looks good to me, at least parts which I understand.
>>>
>>> I would prefer JavaObjectsInPerm be diagnostic flag so we can change its value in product VM. Or experimental since we remove it later, right?
>> JohnC didn't seem to want it to be in the product and I don't have a strong opinion either way. Why don't we leave it develop for now?
>>
>>> vmStructs.cpp: add separation comment for klass_offset:
>>>
>>> /**************************************/
>>> /* java_lang_Class (NOTE: incomplete) */
>>> /**************************************/
>>>
>>> static_field(java_lang_Class, klass_offset, int)
>> I added that along with the rest of the offset fields in case we ever want them later.
>>
>>> it also miss java_lang_Class::hc_number_of_fake_int_fields.
>> The SA doesn't really need to that value so there's no point in adding it. I went ahead and deleted the entry for java_lang_Class::hc_number_of_fake_oop_fields since it's unused.
>>
>> I updated the webrevs. Thanks!
>>
>> tom
>>
>>> Thanks,
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> I discovered that I hadn't tested with CDS on and that required me to change the way that the synthetic fields in Class are laid out. When using the classes from the shared archive the class file parser isn't runtime so the synthetic fields had to be at some well known offset. The current synthetic oop fields are allocated right after the header and I needed 2 new int fields so I chose to allocate them just after the header and have the synthetic oop fields directly follow those. That required some small changes to the SA to use the computed value for klass_offset. The allocation of the Java mirror had to change as well.
>>>> I also brought over support for allocating the mirrors in the normal heap which exposed a few bugs in the oop visitors for arrayKlass. Some of them weren't calling their super visitor so _java_mirror wasn't being visited. arrayKlassKlass::oop_oop_iterate_m was also forwarding to oop_oop_iterate and ignoring the MemRegion.
>>>> I also added a separate flag to control whether the Class is in perm or not instead of overloading ScavengeRootsInCode. I called it JavaObjectsInPerm but I'm open to changing the name. This can also be used for the String changes that Coomes has. It can also be used for debugging once ScavengeRootsInCode is always enabled.
>>>> I reran all my tests and also ran Tony's gc_test_suite with all the various collectors, which showed the oop visitor issues I found. I did testing with -Xshare:dump as well. I've updated the full webrev itself and I also generated a incremental webrev to show what I changed since the last review.
>>>> http://cr.openjdk.java.net/~never/7017732
>>>> http://cr.openjdk.java.net/~never/7017732_incremental
>>>> tom
>>>> On Feb 10, 2011, at 1:07 AM, Christian Thalinger wrote:
>>>>> Yes, looks good. -- Christian
>>>>>
>>>>> On Feb 10, 2011, at 3:40 AM, 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
>
More information about the hotspot-dev
mailing list