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

[Thomas Schatzl]

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.

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

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

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

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

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

The rest seems good.

Thanks,
  Thomas