RFR:8206009: Move java heap object archiving code to heapShared.hpp and heapShared.cpp
Jiangli Zhou
jiangli.zhou at oracle.com
Tue Oct 9 05:29:27 UTC 2018
On 10/5/18 9:17 AM, Ioi Lam wrote:
> Looks good. Thanks!
Thanks. I've merged with the subgraph info hashtable change that you
integrated today. Universe::serialize() is shared by both dump time and
runtime. To be more precise, I also added DumpSharedSpaces to the new
assert:
#ifdef ASSERT
if (DumpSharedSpaces &&
!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
I'll integrate after re-running the tests.
Thanks,
Jiangli
>
> - 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list