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

Ioi Lam ioi.lam at oracle.com
Fri Oct 5 03:16:03 UTC 2018



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

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