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