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