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

[Jiangli Zhou]

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
>>
>>
>