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

[Thomas Schatzl]

Hi,

On Tue, 2017-08-08 at 17:33 -0700, Jiangli Zhou wrote:
> Here is the incremental webrev that has all the changes incorporated
> with suggestions from Coleen, Ioi and Thomas:
> 
> http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.03.inc/
> 
> Updated full webrev: http://cr.openjdk.java.net/~jiangli/8179302/webr
> ev.hotspot.03/
> 

 - (just repeating) g1Allocator.hpp:328:  "Closed" archive region
contain no references outside of archive ... -> outside of other closed
archive regions.
As for the description of open archive regions, it may be useful to
allow references to any other regions (afaik there is no restriction on
pointing to other open archive regions, and none to closed).

 - g1Allocator.inline.hpp:68 and 71: argument alignment.

 - filemap.cpp:473: missing space before the opening bracket

 - filemap.cpp:475: s/consqusective/consecutive

 - g1HeapVerifier.cpp:252: comment should say "Should be closed archive
region"

 - filemap.cpp:675: this is just a note about the big comment. It
mentions "archive string regions" and "string regions" (also seen
"metaspace string region") which may or may not be defined elsewhere.
Maybe it is useful to consolidate these terms.

The text also mentions "regions", which is a G1 specific term. Not sure
if there is a better way to define these areas in this context.

The comment in metaspaceShared.cpp seems to carefully avoid this by
using "shraed strings" and "shared archive heap space" (i.e. no mention
of "regions") most of the time. Sometimes "regions" is also used as
"area". 

Consolidating this may avoid confusion between "G1 regions" and region
as "area" and help a reader.

Or maybe I'm just reading too much into this :)

I have no particular opinion on whether you want to change anything
here.

 - FileMapInfo::map_heap_data(): there are quite a few occurrances of
this construct:

 761     if (log_is_enabled(Info, cds)) {
 762       log_info(cds)("UseSharedSpaces: Unable to allocate region, "

Afaik the log_info() call already only prints out data if
log_is_enabled(Info, cds), so it seems superfluous. The use of
log_is_enabled() call seems only useful if the block inside contains
some expensive computation, which does not seem the case here.

Sorry for bringing up these mostly cosmetic issues that late. I do not
need a re-review for the comment changes (and removal of the
log_is_enabled()).

Thanks,
  Thomas