RFR (L) 8186777: Make Klass::_java_mirror an OopHandle
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Oct 3 20:02:38 UTC 2017
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
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