RFR (L) 8186777: Make Klass::_java_mirror an OopHandle
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Oct 3 20:31:43 UTC 2017
On 10/3/17 4:15 PM, Stefan Karlsson wrote:
> 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 for your help!
Coleen
>
> 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