RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)

[Tom Benson]

Hi Per,
Thanks very much for the review.

On 6/1/2015 10:35 AM, Per Liden wrote:
> Hi Tom,
>
> On 2015-05-29 23:30, Tom Benson wrote:
>> Hi,
>> Please review these changes for JDK-8042668, which constitute the GC
>> support for JDK-8059092 for storing interned strings in CDS archives
>> (JEP 250).  The RFR for JDK-8059092 was recently posted by Jiangli Zhou,
>> and it would be best if overall comments could go to that thread, with
>> GC-specific comments here.
>>
>> JBS:   https://bugs.openjdk.java.net/browse/JDK-8042668
>> Webrev: http://cr.openjdk.java.net/~brutisso/8042668/webrev.00/
>
>
> Maybe it's just me, but the concept of "recording" feels a bit strange 
> in this context. May I suggest that we remove the "record" and 
> "recording" part of the names and instead just call it an "archive" 
> that we can allocate in? Something like:
>
> class G1ArchiveAllocator ... {
>   HeapWord* mem_allocate(...);
>
>   void finalize(...);
>
>   ...
> };
>
> class G1CollectedHeap ... {
>   void begin_archive_mem_allocate();
>
>   bool is_archive_mem_allocation_too_large(...);
>
>   HeapWord* archive_mem_allocate(...);
>
>   void end_archive_mem_allocate(...);
>
>   ...
> };

Hmmm....   Yes, I guess the name "RecordingAllocator" does show the 
evolution of the design, more than the ultimate use.  It was named 
"recording" because it allowed a way to keep track of the recorded 
ranges, in contrast with an earlier design that allocated a block of 
memory up front.   I'm fine with changing this to an ArchiveAllocator as 
you suggest, if I hear no objections.

>
>
> g1CollectedHeap.cpp
> -------------------
>
> * In G1CollectedHeap::end_record_alloc_range(), shouldn't we delete 
> the allocator as the last step?
>

Yes.  I think I made that change at one point and then removed it for 
some reason, which may be gone.  I'll re-make it.


> * I guess this change could be skipped, as it makes the comment 
> slightly malformed.
>
> -        // We ignore humongous regions, we left the humongous set 
> unchanged
> +        // We ignore humongous regions.
> +        // We left the humongous set unchanged,
>

OK.

>
> g1Allocator.hpp
> ---------------
>
> + class G1RecordingAllocator : public CHeapObj<mtGC> {
> +   friend class VMStructs;
>
> You could skip this friend declaration, since it's not accessed by 
> VMStructs. Only needed if the class is exposed in the SA.
>
>

OK.  Thanks,
Tom

> cheers,
> /Per
>
>>
>> These changes add a new "archive" region type to G1.  The description
>> field in JDK-8042668 contains an "Implementation Notes" section which
>> describes components of the design, and should be useful for a code
>> review.   The overview:
>>
>>     "Archive" regions are G1 regions that are not modifiable by GC,
>>     being neither scavenged nor compacted, or even marked in the object
>>     header. They can contain no pointers to non-archive heap regions,
>>     and object headers point to shared CDS metaspace (though this last
>>     point is not enforced by G1). Thus, they allow the underlying
>>     hardware pages to be shared among multiple JVM instances.
>>
>>     In short, a dump-time run (using -Xshare:dump) will allocate space
>>     in the Java heap for the strings which are to be shared, copy the
>>     string objects and arrays to that space, and then archive the entire
>>     address range in the CDS archive. At restore-time (using
>>     -Xshare:on), that same heap range will be allocated at JVM init
>>     time, and the archived data will be mmap'ed into it. GC must treat
>>     the range as 'pinned,' never moving or writing to any objects within
>>     it, so that cross-JVM sharing will work.
>>
>> Testing:  All testing for JDK-8059092 included this code. Manual
>> testing with prototype calls to the new GC support was performed before
>> integration, along with JPRT and benchmark runs.
>>
>> Performance:  The GC changes had no significant impact on SpecJBB, JVM,
>> or Dacapo benchmarks, run on x64 Linux.  However, a small (~1%) increase
>> in Full GC times was seen in tests when the shared string support was
>> not in use, when runs are configured to encounter them.   When shared
>> strings ARE in use, the impact could be as high as 5% for a likely
>> worst-case.   Please see the JBS entry for a discussion.
>>
>> Thanks,
>> Tom
>>