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

[Ioi Lam]

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