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