RFR 8042668: GC Support for shared heap ranges in CDS (RE: JDK-8059092)
Jiangli Zhou
jiangli.zhou at oracle.com
Mon Jun 8 20:18:14 UTC 2015
Thanks, Coleen!
Jiangli
On Jun 8, 2015, at 3:24 AM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>
> 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/.
>>
>> On Jun 5, 2015, at 3:43 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>> 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?
>>
>> Removed.
>>
>>>
>>> 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.
>>
>> 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
>>>
>>> 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
>>>
>>> + 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
>>>
>>> + 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/
>>>>
>>>> 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
>>>>>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list