RFR (S) 8249938: Move mirror oops from Universe into OopStorage

[David Holmes]

Hi Coleen,

This all seems fine to me.

Thanks,
David

On 24/07/2020 11:43 am, coleen.phillimore at oracle.com wrote:
> 
> For the record and for reviewer #2, this is the incremental webrev, with 
> a bug fixed in ReadClosure::do_oop.
> 
> incr webrev at 
> http://cr.openjdk.java.net/~coleenp/2020/8249938.02.incr/webrev
> full webrev at http://cr.openjdk.java.net/~coleenp/2020/8249938.02/webrev
> 
> thanks,
> Coleen
> 
> 
> On 7/23/20 5:49 PM, coleen.phillimore at oracle.com wrote:
>>
>> Thank you for reviewing, Ioi.
>>
>> On 7/23/20 4:41 PM, Ioi Lam wrote:
>>>
>>>
>>> On 7/23/20 10:05 AM, coleen.phillimore at oracle.com wrote:
>>>> Summary: Save and restore mirror oops to temporary array for CDS, 
>>>> and move them to OopStorage once restored.
>>>>
>>>> This is a subtask of moving oops out of Universe.  I ran performance 
>>>> tested of this and there is no performance change. Some slight 
>>>> decrease in number of instructions (improvement!) in 
>>>> Perfstartup-Noop that were flagged as significant - 0.10%
>>>>
>>>> Tested with mach5 tier1-3.
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2020/8249938.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8249938
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>> Hi Coleen,
>>>
>>> The changes looks good. I think Universe::serialize() can be 
>>> simplified as (not tested):
>>>
>>> void Universe::serialize(SerializeClosure* f) {
>>>
>>> #if INCLUDE_CDS_JAVA_HEAP
>>>   {
>>>     oop mirror_oop;
>>>     for (int i = T_BOOLEAN; i < T_VOID+1; i++) {
>>>       if (f->is_reading()) {   // f->reading() ...
>>>         f->do_oop(&mirror_oop); // read from archive
>>>         _mirrors[i] = OopHandle(vm_global(), mirror_oop);
>>>       } else {
>>>         mirror_oop = _mirrors[i].resolve();
>>>         f->do_oop(&mirror_oop); // write to archive
>>>       }
>>>       if (mirror_oop != NULL) { // may be null if archived heap is 
>>> disabled
>>> java_lang_Class::update_archived_primitive_mirror_native_pointers(mirror_oop); 
>>>
>>>       }
>>>     }
>>>   }
>>> #endif
>>
>> Yes that works and is better.  I think GC hates when the location of 
>> mirror_oop is the same, but serializing doesn't care.  I've rerun 
>> tier1 tests on it and it all passes.
> 
> 
>> Thanks!
>> Coleen
>>>
>>> Thanks
>>> - Ioi
>>
>