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

[coleen.phillimore at oracle.com]

On 7/24/20 10:48 PM, Ioi Lam wrote:
>
>
> On 7/24/20 7:22 PM, David Holmes wrote:
>> 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". ??
>>
>
> CDS "serialize" is used for both read and writing. It's probably a bad 
> name but has been there since the beginning of CDS ....

Generally, serialize doesn't have to test whether it's reading or 
writing.  It's at least a useful name to find in the sources.

Thanks for answering David's question!
Coleen
>
> Thanks
> - Ioi
>
>> 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
>>>>>
>>>>>
>>>
>