RFR: 8179302: Pre-resolve constant pool string entries and cache resolved_reference arrays in CDS archive

[Thomas Schatzl]

Hi,

On Thu, 2017-08-03 at 17:15 -0700, Jiangli Zhou wrote:
> Here are the updated webrevs.
> 
> http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/
> http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.02/
> 
> Changes in the updated webrevs include:
> Merge with Ioi’s recent shared space auto-sizing change (8072061)
> Addressed all feedbacks from Ioi and Coleen (Thanks for detailed
> review!)

- the comment in g1Allocator.hpp:326 needs to be updated. I would merge
the information from the _open member here. I.e. define what "open" and
"closed" archive are.

- g1Allocator.inline.hpp:66-72: formatting - parameters should be
aligned below each other

- g1CollectedHeap.cpp:665/6: formatting - parameters should be aligned
below each other

- g1CollectedHeap.cpp:750 + 756: maybe make a static method to avoid
repetition here.

- G1NoteEndOfConcMarkClosure::doHeapRegion(): would it be too much work
to make an extra CR out of this change? It is a change that fixes an
existing bug unrelated to this change after all (not doing remembered
set cleanup work for archive regions).

- g1HeapVerifier.cpp:63: I think this change is obsolete since
is_obj_dead_cond() will always return false. (I.e. some leftover of
some internal discussion)

- g1HeapVerifier.cpp: there is a verbose flag passed around. Not sure
if it should be kept, as it seems to be some code that has been used
for debugging this feature, but can't be activated anyway without code
changes.

- heapRegion.inline.hpp:120: please fix the comment of the assert.
Something like "Closed archive regions should not have references into
other regions"

- heapRegion.inline.hpp:125: I think the existing code of faking open
archive regions as all-live does not work as implemented. Consider the
case when a new object in there is made live, and references in there
set to refer to some object outside this region, and is the only
reference (and it has not been marked live yet): if there is a
remembered set entry to that, and it is about to be scanned.

The current implementation of HeapRegion::is_obj_dead() will consider
it dead, so we will enter the code path at line 125. Block_size_using
bitmap() will jump over that object, but the return values of
is_obj_dead_with_size() method will indicate the caller to not iterate
over this object anyway, potentially missing that reference.

HeapRegion::is_obj_dead() needs to return that the object is not dead
for open archive regions. I think for now the safest way is to add
!is_open_archive() to the condition calculated there. That will obviate
the need for that existing hack to the assert too.

It may have some perf impact though - actually recently there has been
some effort to remove that is_archive() check from that code (the one
that is now the is_closed_archive() assert). I do not see an easy way
to fix this. :( (i.e. there is likely no perf impact vs. jdk9 so it's
not that bad)

This suggestion also only works with the assumption laid out in the CR
that there is no way that a live object can not become dormant again,
and the objects in the open archive regions are always parsable (never
contain junk data).

- HeapRegionType::relabel_as_old(): the code in line 175/176 is
unreachable and should be removed.

I did not look through runtime code too closely.

Thanks,
  Thomas