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

[Jiangli Zhou]

Hi Thomas,

Thank you so much for looking at the update in great detail! All points were taken and fixed as suggested.

> On Aug 9, 2017, at 8:12 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> 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).

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

I changed the comments and replaced with shared string object and open archive heap objects (or open archive heap data).

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

Removed those unnecessary log_is_enabled(Info, cds) checks.

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

Jiangli
> 
> Thanks,
>   Thomas