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

Ioi Lam ioi.lam at oracle.com
Tue Aug 21 17:38:33 UTC 2018

Hi Thomas.

Thanks for the review. I've updated the webrev according to your 
comments. See


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


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


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


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


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

- Ioi

> The rest seems good.
> Thanks,
>    Thomas

More information about the hotspot-runtime-dev mailing list