RFR (T) 8250519: [REDO] Move mirror oops from Universe into OopStorage

[David Holmes]

Hi Ioi,

On 25/07/2020 9:59 am, Ioi Lam wrote:
> 
> 
> On 7/24/20 4:52 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> On 25/07/2020 7:07 am, coleen.phillimore at oracle.com wrote:
>>> Summary: The original patch but add a null pointer check so an 
>>> OopHandle is not created if a NULL is read from the archive.
>>>
>>> If a NULL is read from the archive, we shouldn't create an OopHandle 
>>> for it because one will be created in initialize_basic_type_mirrors. 
>>> The assert added in the previous patch I pushed detected that.
>>
>> Your fix to skip NULL seems reasonable but I'm left confused as to 
>> when and why we will find NULL for these basic type mirrors. Is it 
>> only the reference types that will be NULL?
>>
> 
> Hi David,
> 
> The null skipping happens here:
> 
>   263       if (f->reading()) {
>   264         f->do_oop(&mirror_oop); // read from archive
>   265         assert(oopDesc::is_oop_or_null(mirror_oop), "is oop");
>   266         // Only create an OopHandle for non-null mirrors
>   267         if (mirror_oop != NULL) {
>   268           _mirrors[i] = OopHandle(vm_global(), mirror_oop);
>   269         }
> 
> where the f->do_oop() will read an archived oop from the CDS heap. There 
> are cases where the a NULL is returned inside mirror_oop:
> 
> (1) The CDS image was created WITHOUT an archived heap (this could 
> happen when you run -Xshare:dump with a GC or compressed oop encoding 
> that's not compatible with the archived heap).
> 
> (2) The CDS image has an archived heap, but the current VM is using a GC 
> or compressed oop encoding that's not compatible with the archived heap.

Thanks for the explanation. I'm having trouble seeing the equivalence of 
the old Universe::serialize code and the new version. The new version 
seems to perform both writing and reading - which seem odd for something 
called "serialize". ??

Thanks,
David
-----

> Thanks
> - Ioi
> 
> 
> 
>> Thanks,
>> David
>>
>>> incremental change to original change: 
>>> http://cr.openjdk.java.net/~coleenp/2020/8250519.01.incr/webrev
>>> full webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2020/8250519.01/webrev
>>>
>>> Retested with tier1-3.
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>