RFR (L) 8186777: Make Klass::_java_mirror an OopHandle

[Erik Osterlund]

Hi Coleen,

> On 2 Oct 2017, at 19:04, coleen.phillimore at oracle.com wrote:
> 
> 
> 
>> On 10/2/17 12:48 PM, Erik Osterlund wrote:
>> Hi Coleen,
>> 
>> I looked a bit at the code generation part of this change.
>> It beats me that the indirect load required for resolution of the oop handle was somewhat encapsulated in a resolve oop handle call in the macro assembler (a bit like resolve jobject), but in the corresponding C1 and C2 code, there is no such abstraction. Instead the loads required for resolve are generated straight up. Therefore, if the logic involved in resolving an OopHandle ever changes, it might start to get tricky to chase down where it is being used too.
> 
> Hi Erik,  I wanted the load encaspulated in resolve_oop_handle() in the macroAssembler, but I didn't know how to change the c1/c2 code (or graal) to do the same.
>> 
>> So I wonder if you would find it useful to encapsulate that into some method on e.g. LIRGenerator for C1 and GraphKit for C2?
>> In the case of C2 it might be a bit tricky to abstract due to the node matching logic, unless we want to macro expand a new ResolveOopHandleNode, or something like that. Or a matching function maybe.
> 
> Can I file a seperate RFE for this?  I like the idea very much but would like to push this larger change first.

Sure, I am fine with that.

>> 
>> Just a thought that beat me reading through the changes. I like abstractions!
> 
> Me too!

:)

Thanks,
/Erik

> Thanks,
> Coleen
>> 
>> Thanks,
>> /Erik
>> 
>>> On 28 Sep 2017, at 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
>