RFR(S) 8208658: Make CDS archived heap regions usable even if compressed oop encoding has changed

[Ioi Lam]

Hi Jiangli,

Thanks for spotting this. I will fix it as you suggested.

Thanks

- Ioi


On 8/21/18 8:07 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> One minor issue below:
>
>  937   if (narrow_klass_base() != Universe::narrow_klass_base() ||
>  938       narrow_klass_shift() != Universe::narrow_klass_shift()) {
>  939     log_info(cds)("CDS heap data cannot be used because the 
> archive was created with an incompatible oop encoding mode.");
>  940     return;
>  941   }
>
> The 'oop encoding mode' in the above log should be 'narrow kalss 
> encoding mode'. Other than that, the update looks good. No new webrev 
> is needed for me with the log message fix.
>
> Thanks,
>
> Jiangli
>
>
> On 8/21/18 10:38 AM, Ioi Lam wrote:
>> Hi Thomas.
>>
>> Thanks for the review. I've updated the webrev according to your 
>> comments. See
>>
>> http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-heap-regions.v03.delta/ 
>>
>>
>> More comments in-line.
>>
>>
>> On 8/21/18 2:23 AM, Thomas Schatzl wrote:
>>> Hi,
>>>
>>> On Mon, 2018-08-20 at 17:32 -0700, Ioi Lam wrote:
>>>> Hi Jiangli,
>>>>
>>>> Thanks for the review. I've updated the webrev at:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-hea
>>>> p-regions.v02/
>>>>
>>>> Delta from the last webrev:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-hea
>>>> p-regions.v02.delta/
>>>>
>>>>
>>>> [...]
>>>> On 8/17/2018 11:49 AM, Jiangli Zhou wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> Thanks a lot for the updated webrev! Please see some more
>>>>> comments/suggestions below.
>>>>>
>>>>> - src/hotspot/share/memory/filemap.cpp
>>>>>
>>>>>   195   if (MetaspaceShared::is_heap_object_archiving_allowed()) {
>>>>>   196     _g1_reserved = G1CollectedHeap::heap()->g1_reserved();
>>>>>   197   }
>>>>>
>>>>> The above are G1 specific. It should be included under #if
>>>>> INCLUDE_G1GC.
>>>>>
>>>> Fixed. I tested with "configure --with-jvm-features=-g1gc ...". I
>>>> ended up having to fix a couple of JFR files as well.
>>> - In jfrPeriodic.hpp, I would #ifdef the whole
>>> VM_G1SendHeapRegionInfoEvetns class and its use. There does not seem to
>>> be a point to actually compile in all the code to execute the VM
>>> operation.
>>>
>>> Probably I recommend to extract the changes to the JFR events into a
>>> separate RFR, and send the rfr to the jfr people to look at too.
>>
>> I have filed JDK-8209802 and reverted my changes in this RFE
>>
>>> - extra newline in filemap.cpp:858
>>
>> Fixed
>>
>>>
>>> - in filemap.cpp:877 I can not parse the comment to
>>> FileMapInfo::get_heap_regions_range_with_current_oop_encoding_mode.
>>>
>>> Maybe "Returns the address range that contains heap regions with oops
>>> in the current oop encoding mode"?
>>>
>>> In that case, the comment does not seem to say anything more than the
>>> method name and could be skipped.
>>>
>>> Or "returns a (new) address range that can be used by heap regions
>>> using the current oop encoding mode?"
>>
>> How about this?
>>
>> // Returns the address range of the archived heap regions computed 
>> using the
>> // current oop encoding mode. This range may be different than the 
>> one seen at
>> // dump time due to encoding mode differences. The result is used in 
>> determining
>> // if/how these regions should be relocated at run time.
>>
>>
>>> - maybe improve the comment in filemap.cpp:905:
>>>
>>> "The shared strings are mapped near the runtime java heap top" -> "The
>>> shared strings are mapped close to the end of the java heap top in
>>> closed archive regions"
>>
>> Done
>>
>>> - the info messages about "incompatible heap size" should be adapted in
>>> lines 940 and 947. It's not heap size that is incompatible, but klass
>>> encoding and narrow oop encoding configuration respectively now.
>>>
>>> That would make the messages more useful imho.
>>
>> I changed to "because ... an incompatible oop encoding mode."
>>
>>> - the code to manually calculate the delta in line 970-972 should
>>> probably be replaced by calling the pointer_delta() utility method.
>>> (The code is not wrong, just seems a bit verbose)
>>
>> I can't use pointer_delta, because it returns a size_t which is 
>> unsigned.
>> In my case, delta can be positive or negative. I looked but couldn't
>> find a signed version of pointer_delta.
>>
>>> - filemap.cpp:952-954: I may be wrong here, but this info message is
>>> split across three lines in the log, while all other messages so far
>>> are single line output.
>>
>> That was intentional. A few logs above were also split into several 
>> lines.
>>
>> When debugging, I found it much easier to read than a very long line
>> that gets wrapped arbitrarily by the terminal.
>>
>>> - filemap.cpp:1042: I think the "size" could be printed using the
>>> SIZE_FORMAT_W macro instead of casting to int and inventing your own
>>> format specifier.
>>>
>>> - filemap.cpp:1077: use SIZE_FORMAT instead of UINTX_FORMAT
>>
>> Fixed.
>>
>>> - filemap.hpp:155: maybe not use a member called "_g1_reserved" in
>>> anticipation of other collectors implementing this feature. Something
>>> like "_reserved" would be fine; also CollectedHeap afaik already
>>> provides the reserved_memory() getter which can be used independently
>>> of the GC to determine that area.
>>>
>>> This does not mean that AppCDS will suddenly work with all collectors.
>>> Just that the person doing this work later does not have to change this
>>> as well.
>>
>> Fixed.
>>
>>> - metaspaceshared.cpp:1844: please consider using
>>> SIZE_FORMAT/SIZE_FORMAT_W as format specifiers for the sizes in that
>>> log message.
>>>
>>> (I only checked the changed log messages for format specifiers, there
>>> may be more elsewhere btw)
>>
>> I scanned the filemap.cpp and metaspaceshared.cpp and changed a
>> few more places to use SIZE_FORMAT
>>
>> Thanks
>> - Ioi
>>
>>> The rest seems good.
>>>
>>> Thanks,
>>>    Thomas
>>>
>>>
>>
>