RFR (L) 8186777: Make Klass::_java_mirror an OopHandle
Stefan Karlsson
stefan.karlsson at oracle.com
Tue Oct 3 20:15:06 UTC 2017
On 2017-10-03 22:02, coleen.phillimore at oracle.com wrote:
>
> Stefan found a problem that set_java_mirror() code could be unsafe if
> the java_mirror code changes, which the function allowed one to do.
> There is code in jvmtiRedefineClasses that temporarily switches the
> java_mirrors for verification of the newly loaded class. Since this
> simply swaps java_mirrors that are together in the
> ClassLoaderData::_handles area, I added an API for that and made
> set_java_mirror() more restrictive.
>
> I reran JVMTI, CDS and tier1 tests. New webrev with all changes are:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8186777.04/webrev
The GC parts look good to me.
Thanks,
StefanK
>
> Thanks,
> Coleen
>
> On 10/3/17 10:23 AM, coleen.phillimore at oracle.com wrote:
>>
>> Here is an updated webrev with fixes for your comments.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8186777.03/webrev
>>
>> Thanks for reviewing and all your help with this!
>>
>> Coleen
>>
>> On 9/29/17 6:41 AM, Stefan Karlsson wrote:
>>> Hi Coleen,
>>>
>>> I started looking at this, but will need a second round before I've
>>> fully reviewed the GC parts.
>>>
>>> Here are some nits that would be nice to get cleaned up.
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/classfile/classLoaderData.cpp.frames.html
>>>
>>>
>>> 788 record_modified_oops(); // necessary?
>>>
>>> This could be removed. Only G1 cares about deleted "weak" references.
>>>
>>> Or we can wait until ErikÖ's GC Barrier Interface is in place and
>>> remove it then.
>>>
>>> ----------
>>>
>>> #ifdef CLD_DUMP_KLASSES
>>> if (Verbose) {
>>> Klass* k = _klasses;
>>> while (k != NULL) {
>>> - out->print_cr("klass " PTR_FORMAT ", %s, CT: %d, MUT: %d", k,
>>> k->name()->as_C_string(),
>>> - k->has_modified_oops(), k->has_accumulated_modified_oops());
>>> + out->print_cr("klass " PTR_FORMAT ", %s", k,
>>> k->name()->as_C_string());
>>> assert(k != k->next_link(), "no loops!");
>>> k = k->next_link();
>>> }
>>> }
>>> #endif // CLD_DUMP_KLASSES
>>>
>>> Pre-existing: I don't think this will compile if you turn on
>>> CLD_DUMP_KLASSES. k must be p2i(k).
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/classfile/classLoaderData.hpp.udiff.html
>>>
>>>
>>> + // Remembered sets support for the oops in the class loader data.
>>> + jbyte _modified_oops; // Card Table Equivalent
>>> (YC/CMS support)
>>> + jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS
>>> support)
>>>
>>> We should create a follow-up bug to change these jbytes to bools.
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1HeapVerifier.cpp.frames.html
>>>
>>>
>>> Spurious addition:
>>> + G1CollectedHeap* _g1h;
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/g1/g1OopClosures.hpp.udiff.html
>>>
>>>
>>> Spurious addition?:
>>> + G1CollectedHeap* g1() { return _g1; }
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/parallel/psScavenge.inline.hpp.patch
>>>
>>>
>>> PSPromotionManager* _pm;
>>> - // Used to redirty a scanned klass if it has oops
>>> + // Used to redirty a scanned cld if it has oops
>>> // pointing to the young generation after being scanned.
>>> - Klass* _scanned_klass;
>>> + ClassLoaderData* _scanned_cld;
>>>
>>> Indentation.
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/parallel/psTasks.cpp.frames.html
>>>
>>>
>>> 80 case class_loader_data:
>>> 81 {
>>> 82 PSScavengeCLDClosure ps(pm);
>>> 83 ClassLoaderDataGraph::cld_do(&ps);
>>> 84 }
>>>
>>> Would you mind changing the name ps to cld_closure?
>>>
>>> ==========
>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/src/hotspot/share/gc/shared/genOopClosures.hpp.patch
>>>
>>>
>>> + OopsInClassLoaderDataOrGenClosure* _scavenge_closure;
>>> // true if the the modified oops state should be saved.
>>> bool _accumulate_modified_oops;
>>>
>>> Indentation.
>>>
>>> ----------
>>> + void do_cld(ClassLoaderData* k);
>>>
>>> Rename k?
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2017-09-28 23:36, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Thank you to Stefan Karlsson offlist for pointing out that the
>>>> previous .01 version of this webrev breaks CMS in that it doesn't
>>>> remember ClassLoaderData::_handles that are changed and added while
>>>> concurrent marking is in progress. I've fixed this bug to move the
>>>> Klass::_modified_oops and _accumulated_modified_oops to the
>>>> ClassLoaderData and use these fields in the CMS remarking phase to
>>>> catch any new handles that are added. This also fixes this bug
>>>> https://bugs.openjdk.java.net/browse/JDK-8173988 .
>>>>
>>>> In addition, the previous version of this change removed an
>>>> optimization during young collection, which showed some uncertain
>>>> performance regression in young pause times, so I added this
>>>> optimization back to not walk ClassLoaderData during young
>>>> collections if all the oops are old. The performance results of
>>>> SPECjbb2015 now are slightly better, but not significantly.
>>>>
>>>> This latest patch has been tested on tier1-5 on linux x64 and
>>>> windows x64 in mach5 test harness.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/8186777.02/webrev/
>>>>
>>>> Can I get at least 3 reviewers? One from each of the compiler, gc,
>>>> and runtime group at least since there are changes to all 3.
>>>>
>>>> Thanks!
>>>> Coleen
>>>>
>>>>
>>>> On 9/6/17 12:04 PM, coleen.phillimore at oracle.com wrote:
>>>>> Summary: Add indirection for fetching mirror so that GC doesn't
>>>>> have to follow CLD::_klasses
>>>>>
>>>>> Thank you to Tom Rodriguez for Graal changes and Rickard for the
>>>>> C2 changes.
>>>>>
>>>>> Ran nightly tests through Mach5 and RBT. Early performance
>>>>> testing showed good performance improvment in GC class loader data
>>>>> processing time, but nmethod processing time continues to
>>>>> dominate. Also performace testing showed no throughput
>>>>> regression. I'm rerunning both of these performance testing and
>>>>> will post the numbers.
>>>>>
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8186777
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8186777.01/webrev
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>
>
More information about the hotspot-dev
mailing list