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

Jiangli Zhou jiangli.zhou at oracle.com
Fri Oct 5 05:55:23 UTC 2018


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
>>>>
>>>>
>>>>
>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list