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

[Jiangli Zhou]

Here is the updated runtime webrev reflects the name changes: http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/

Thanks,
JIangli

On Jun 2, 2015, at 4:39 AM, Tom Benson <tom.benson at oracle.com> wrote:

> Hi,
> An updated webrev addressing the comments from Per and Bengt is athttp://cr.openjdk.java.net/~brutisso/8042668/webrev.01/ .
> I also updated the notes in the JBS entry to reflect the name changes.
> Tom
> 
> On 6/1/2015 11:22 AM, Tom Benson wrote:
>> 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
>>>> 
>> 
>