RFR:8206009: Move java heap object archiving code to heapShared.hpp and heapShared.cpp

[Ioi Lam]

Looks good. Thanks!

- Ioi


On 10/4/18 10:55 PM, Jiangli Zhou wrote:
> On 10/4/18 8:16 PM, Ioi Lam wrote:
>
>>
>>
>> On 10/4/18 11:00 AM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>> Thanks for the review.
>>>
>>> On 10/4/18 8:51 AM, Ioi Lam wrote:
>>>> Hi Jiangli,
>>>>
>>>> The fix looks good.
>>>>
>>>> #if INCLUDE_CDS_JAVA_HEAP
>>>>   // The mirrors are NULL if 
>>>> HeapShared::is_heap_object_archiving_allowed
>>>>   // is false.
>>>>   f->do_oop(&_int_mirror);
>>>>   f->do_oop(&_float_mirror);
>>>>
>>>> Maybe the comment should be turned into an assert?
>>> There are existing asserts in 
>>> Universe::initialize_basic_type_mirrors() already that verify the 
>>> primitive type mirrors are not NULL when 
>>> HeapShared::is_heap_object_archiving_allowed is true.
>>>
>>>     if (UseSharedSpaces &&
>>>         HeapShared::open_archive_heap_region_mapped() &&
>>>         _int_mirror != NULL) {
>>>       assert(HeapShared::is_heap_object_archiving_allowed(), "Sanity");
>>>       assert(_float_mirror != NULL && _double_mirror != NULL &&
>>>              _byte_mirror  != NULL && _byte_mirror != NULL &&
>>>              _bool_mirror  != NULL && _char_mirror != NULL &&
>>>              _long_mirror  != NULL && _short_mirror != NULL &&
>>>              _void_mirror  != NULL, "Sanity");
>>
>> But these asserts are the opposite of what the comment says: "The 
>> mirrors are NULL if HeapShared::is_heap_object_archiving_allowed is 
>> false." The existing asserts are "A implies B". I think it's better 
>> to also add "not A implies not B".
> I replaced with the following assert in Universe::serialize():
>
> #ifdef ASSERT
>   if (!HeapShared::is_heap_object_archiving_allowed()) {
>     assert(_int_mirror == NULL    && _float_mirror == NULL &&
>               _double_mirror == NULL && _byte_mirror == NULL  &&
>               _bool_mirror == NULL   && _char_mirror == NULL  &&
>               _long_mirror == NULL   && _short_mirror == NULL &&
>               _void_mirror == NULL, "mirrors should be NULL");
>   }
> #endif
>
> Here is the updated webrev: 
> http://cr.openjdk.java.net/~jiangli/8206009/webrev.02/src/hotspot/share/memory/universe.cpp.frames.html
>
> Thanks,
>
> Jiangli
>>
>> Thanks
>> - Ioi
>>
>>>>
>>>> Also, have you tested with configure --disable-precompiled-headers?
>>> Build with --disable-precompiled-headers works without failure on 
>>> linux-x64.
>>>
>>> I will queue my change after your subgraph info hash table change 
>>> for 8210388 and resolve conflicts.
>>>
>>> Thanks,
>>> Jiangli
>>>>
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>>
>>>>
>>>> On 10/3/18 3:23 PM, Jiangli Zhou wrote:
>>>>> Please review the restructuring and cleanup of java heap object 
>>>>> archiving code. The java object archiving code has grown in the 
>>>>> past year and metaspaceShared.* files are not the suitable place. 
>>>>> The restructuring and cleanup include:
>>>>>
>>>>>   - Moved java heap object archiving implementation from 
>>>>> metaspaceShared.* to heapShared.*.
>>>>>   - Various is_archive_object() APIs are renamed to 
>>>>> is_archived_object() for naming consistency:
>>>>>      - Renamed MetaspaceShared::is_archive_object() to 
>>>>> HeapShared::is_archived_object().
>>>>>      - Renamed oopDesc::is_archive_object() to 
>>>>> oopDesc::is_archived_object().
>>>>>      - Renamed G1ArchiveAllocator::is_archive_object() to 
>>>>> G1ArchiveAllocator::is_archived_object().
>>>>>   - Changed to use G1ArchiveAllocator::is_archived_object() in 
>>>>> G1CollectedHeap::materialize_archived_object(). Removed #include 
>>>>> "memory/metaspaceShared.inline.hpp” from g1CollectedHeap.cpp.
>>>>>   - Renamed HeapShared::archive_static_fields() to 
>>>>> HeapShared::archive_object_subgraphs().
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8206009/webrev.00/
>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8206009
>>>>>
>>>>> Tested with tier1-tier3. Tier4 and tier5 are in progress.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jiangli
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>