RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Jun 8 10:24:49 UTC 2015
Jiangli, The new comment and webrev look good.
thanks,
Coleen
On 6/7/15 5:15 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
> Thank you for review! Here is an updated webrev,
> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.03/
> <http://cr.openjdk.java.net/%7Ejiangli/8059092/webrev_hotspot.03/>.
>
> On Jun 5, 2015, at 3:43 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>>
> wrote:
>
>> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/classfile/javaClasses.hpp.udiff.html
>> <http://cr.openjdk.java.net/%7Ejiangli/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?
>
> Removed.
>
>>
>> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/classfile/stringTable.cpp.udiff.html
>> <http://cr.openjdk.java.net/%7Ejiangli/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.
>
> I rename the function to lookup_in_main_table(). It sounds good to me.
>
>>
>> + oop s = (oop)(bucket->literal());
>>
>>
>> Is this cast unnecessary since literal() is an oop in this table?
>
> Removed.
>
>>
>>
>> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/gc/g1/g1StringDedupThread.cpp.udiff.html
>> <http://cr.openjdk.java.net/%7Ejiangli/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?
>
>
> I added following comments:
>
> // The CDS archive does not include the string dedupication table.
> Only the string
> // table is saved in the archive. The shared strings from CDS archive
> need to be
> // added to the string dedupication table before deduplication occurs.
> That is
> // done in the begining of the G1StringDedupThread (see
> G1StringDedupThread::run()
> // below).
>
>>
>> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/memory/filemap.cpp.udiff.html
>> <http://cr.openjdk.java.net/%7Ejiangli/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).
>
> Done.
>
>>
>> + addr = _header->region_addr(i);
>>
>>
>> Same with adde.
>
> Done.
>
>>
>> http://cr.openjdk.java.net/~jiangli/8059092/webrev_hotspot.02/src/share/vm/memory/filemap.hpp.udiff.html
>> <http://cr.openjdk.java.net/%7Ejiangli/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?
>
> I thought that would look neater. ;) I removed the extra spaces before
> _narrow_oop_mode and also added a comment for the field.
>
>>
>> This whole change looks really good. My comments are minor.
>
> Thanks!
>
> Jiangli
>>
>> 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/
>>> <http://cr.openjdk.java.net/%7Ejiangli/8059092/webrev_hotspot.02/>
>>>
>>> Thanks,
>>> JIangli
>>>
>>> On Jun 2, 2015, at 4:39 AM, Tom Benson <tom.benson at oracle.com
>>> <mailto: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/
>>>> <athttp://cr.openjdk.java.net/%7Ebrutisso/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/
>>>>>>> <http://cr.openjdk.java.net/%7Ebrutisso/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
>>>>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list