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

Fri Jun 5 22:43:17 UTC 2015

http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/classfile/javaClasses.hpp.udiff.html

+    string->obj_field_put_raw(value_offset,  (oop)buffer);

Do you need the oop cast since objArrayOop is a subclass of oop?

http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/classfile/stringTable.cpp.udiff.html

Can you change the name lookup_dynamic to lookup_runtime() instead? I 
think invokedynamic when I see this or some other dynamic sort of 
thing.  It's just the runtime string table, right?   Or 
lookup_in_main_table() which is longer but it's mostly hidden.

+      oop s = (oop)(bucket->literal());

Is this cast unnecessary since literal() is an oop in this table?

http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/gc/g1/g1StringDedupThread.cpp.udiff.html

Can you add a comment why you are deduplicating the shared strings? and 
when this is happening?   Is this at startup to prime the deduplication 
table?

http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/memory/filemap.cpp.udiff.html

+  buf = _header->region_addr(i);

Can you make this statement intialize buf (move the type declaration to 
this line).

+  addr = _header->region_addr(i);

Same with addr.

http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/memory/filemap.hpp.udiff.html

+    int    _narrow_oop_shift;         // compressed oop encoding shift
+    uintx  _max_heap_size;            // java max heap size during dumping
+    Universe::NARROW_OOP_MODE         _narrow_oop_mode;

Can you make _narrow_oop_mode not line up with the comments?

This whole change looks really good.   My comments are minor.

The name change from record to archive looks a lot better!

Thanks,
Coleen

On 6/2/15 3:33 PM, Jiangli Zhou wrote:
> 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
>>>>>