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

[Tom Benson]

Hi Bengt,
Thanks very much for the review.

On 6/1/2015 4:35 AM, Bengt Rutisson wrote:
>
> Hi Tom,
>
> (Correct email thread this time, I hope.)
>
> Thanks for providing the great summary that eased the review process. 
> This change looks good to me.
>
> Some minor comments:
>
> g1Allocator.cpp
>
>  214   assert(_bottom >= _allocation_region->bottom(), "inconsistent 
> allocation state");
>  215   assert(_max <= _allocation_region->end(), "inconsistent 
> allocation state");
>  216   assert(_bottom <= old_top && old_top <= _max, "inconsistent 
> allocation state");
>
> Maybe add err_msg() to see what the values actually were?
>

Good idea.

>
> g1CollectedHeap.cpp
>
>  965 bool
>  966 G1CollectedHeap::check_archive_addresses()
>
> We usually don’t have a linefeed after the return type. So, this 
> should be:
>
>  965 bool G1CollectedHeap::check_archive_addresses()
>
> Same for G1CollectedHeap::alloc_archive_regions() and 
> G1CollectedHeap::fill_archive_regions()
>
>

OK.  I made the mistake of following the example of existing allocation 
routines.   8^)

> g1MarkSweep.cpp
>
>  306 void G1MarkSweep::enable_archive_object_check() {
>  307   if (!_archive_check_enabled) {
>
> Shouldn’t this be called exactly once, either from 
> G1RecordingAllocator::create_allocator() or 
> G1CollectedHeap::alloc_archive_regions()? In that case maybe the “if” 
> should be changed to an assert or guarantee that 
> !_archive_check_enabled holds.
>

I think this is debatable, but I'll change it.   As it is now, 
alloc_archive_regions could be called multiple times to map multiple 
archived MemRegion sets.  In this current use for CDS share strings, 
there should indeed be only 1 call.   When I change this to an 
assertion, that will be the only thing preventing multiple calls, should 
another use of archive regions be added.   The assertion can be removed 
then.
Tom

> Thanks,
> Bengt
>
>
> On 29/05/15 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/
>>
>> 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
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150601/8d517f86/attachment.htm>