RFR: 8179302: Pre-resolve constant pool string entries and cache resolved_reference arrays in CDS archive
Ioi Lam
ioi.lam at oracle.com
Fri Aug 4 21:22:30 UTC 2017
Hi Jiangli,
The code looks good in general. I just have a few pet peeves for
readability:
(1) stringTable.cpp and metaspaceShared.cpp have the same asserts
704 assert(UseG1GC, "Only support G1 GC");
705 assert(UseCompressedOops && UseCompressedClassPointers,
706 "Only support UseCompressedOops and
UseCompressedClassPointers enabled");
1615 assert(UseG1GC, "Only support G1 GC");
1616 assert(UseCompressedOops && UseCompressedClassPointers,
1617 "Only support UseCompressedOops and
UseCompressedClassPointers enabled");
Maybe it's better to combine them into a single function like
MetaspaceShared::assert_vm_flags() so they don't get out of sync?
(2) FileMapInfo::write_archive_heap_regions()
I still find this code very hard to read, especially due to the loop.
First, the comments are not consistent with the code:
498 assert(arr_len <= max_num_regions, "number of memory regions
exceeds maximum");
but the comments says: "The rest are consecutive full GC regions" which
means there's a chance for max_num_regions to be more than 2 (which will
be the case with Calvin's java-loader dumping changes using very small
heap size).So the code is actually wrong.
The word "region" is used in these parameters, but they don't mean the
same thing.
GrowableArray<MemRegion> *regions
int first_region, int max_num_regions,
How about regions -> g1_regions_list
first_region -> first_region_in_archive
In the comments, I find the phrase 'the current archive heap region'
ambiguous. It could be (erroneously) interpreted as "a region from the
currently mapped archive"
To make it unambiguous, how about changing
464 // Write the current archive heap region, which contains one or
multiple GC(G1) regions.
to
// Write the given list of G1 memory regions into the archive,
starting at
// first_region_in_archive.
Also, for the explanation of how the G1 regions are written into the
archive, how about:
// The G1 regions in the list are sorted in ascending address order.
When there are more objects
// than the capacity of a single G1 region, the bottom-most G1
region may be partially filled, and the
// remaining G1 region(s) are consecutively allocated and fully filled.
//
// Thus, the bottom-most G1 region (if not empty) is written into
first_region_in_archive.
// The remaining G1 regions (if exist) are coalesced and written as
a single block
// into (first_region_in_archive + 1)
// Here's the mapping from (g1 regions) -> (archive regions).
All this function needs to do is to decide the values for
r0_start, r0_top
r1_start, r1_top
I think it would be much better to not use the loop, and not use the
max_num_regions parameter (it's always 2 anyway).
*r0_start = *r0_top = NULL;
*r1_start = *r1_top = NULL;
if (arr_len >= 1) {
*r0_start = regions->at(0).start();
*r0_end = *r0_start + regions->at(0).byte_size();
}
if (arr_len >= 2) {
int last = arr_len - 1;
*r1_start = regions->at(1).start();
*r1_end = regions->at(last).start() +
regions->at(last).byte_size();
}
what do you think?
(3) metaspace.cpp
3350 // Map the archived heap regions after compressed pointers
3351 // because it relies on compressed class pointers setting
to work
do you mean this?
// Archived heap regions depend on the parameters of compressed
class pointers, so
// they must be mapped after such parameters have been decided in
the above call.
(4) I found this name not strictly grammatical. How about this:
allow_archive_heap_object -> is_heap_object_archiving_allowed
(5) in most of your code, 'archive' is used as a noun, except in
StringTable::archive_string() where it's used as a verb.
archive_string could also be interpreted erroneously as "return a string
that's already in the archive". So to be consistent and unambiguous, I
think it's better to rename it to StringTable::create_archived_string()
Thanks
- Ioi
On 8/3/17 5:15 PM, Jiangli Zhou wrote:
> Here are the updated webrevs.
>
> http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.02/
> <http://cr.openjdk.java.net/%7Ejiangli/8179302/webrev.hotspot.02/>
> http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.02/
>
> Changes in the updated webrevs include:
>
> * Merge with Ioi’s recent shared space auto-sizing change (8072061)
> * Addressed all feedbacks from Ioi and Coleen (Thanks for detailed
> review!)
>
>
> Thanks,
> Jiangli
>
>
>> On Aug 1, 2017, at 5:29 PM, Jiangli Zhou <jiangli.zhou at Oracle.COM> wrote:
>>
>> Hi Ioi,
>>
>> Thank you so much for reviewing this. I’ve addressed all your
>> feedbacks. Please see details below. I’ll updated the webrev
>> after addressing Coleen’s comments.
>>
>>> On Jul 30, 2017, at 9:07 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>
>>> Hi Jiangli,
>>>
>>> Here are my comments. I've not reviewed the GC code and I'll leave
>>> that to the GC experts :-)
>>>
>>> stringTable.cpp: StringTable::archive_string
>>>
>>> add assert for DumpSharedSpaces only
>>
>> Ok.
>>
>>>
>>> filemap.cpp
>>>
>>> 525 void
>>> FileMapInfo::write_archive_heap_regions(GrowableArray<MemRegion>
>>> *regions,
>>> 526 int first_region,
>>> int num_regions) {
>>>
>>> When I first read this function, I found it hard to follow,
>>> especially this part that coalesces the trailing regions:
>>>
>>> 537 int len = regions->length();
>>> 538 if (len > 1) {
>>> 539 start = (char*)regions->at(1).start();
>>> 540 size = (char*)regions->at(len - 1).end() - start;
>>> 541 }
>>> 542 }
>>>
>>> The rest of filemap.cpp always perform identical operations on
>>> MemRegion arrays, which are either 1 or 2 in size. However,
>>> this function doesn't follow that pattern; it also has a very
>>> different notion of "region", and the confusing part is
>>> regions->size() is not the same as num_regions.
>>>
>>> How about we change the API to something like the following? Before
>>> calling this API, the caller needs to coalesce the trailing
>>> G1 regions into a single MemRegion.
>>>
>>> FileMapInfo::write_archive_heap_regions(MemRegion *regions, int
>>> first, int num_regions) {
>>> if (first == MetaspaceShared::first_string) {
>>> assert(num_regons <= MetaspaceShared::max_strings, "...");
>>> } else {
>>> assert(first ==
>>> MetaspaceShared::first_open_archive_heap_region, "...");
>>> assert(num_regons <=
>>> MetaspaceShared::max_open_archive_heap_region, "...");
>>> }
>>> ....
>>>
>>>
>>
>> I’ve reworked the function and simplified the code.
>>
>>>
>>> 756 if (!string_data_mapped) {
>>> 757 StringTable::ignore_shared_strings(true);
>>> 758 assert(string_ranges == NULL && num_string_ranges == 0,
>>> "sanity");
>>> 759 }
>>> 760
>>> 761 if (open_archive_heap_data_mapped) {
>>> 762 MetaspaceShared::set_open_archive_heap_region_mapped();
>>> 763 } else {
>>> 764 assert(open_archive_heap_ranges == NULL &&
>>> num_open_archive_heap_ranges == 0, "sanity");
>>> 765 }
>>>
>>> Maybe the two "if" statements should be more consistent? Instead of
>>> StringTable::ignore_shared_strings, how
>>> about StringTable::set_shared_strings_region_mapped()?
>>
>> Fixed.
>>
>>>
>>> FileMapInfo::map_heap_data() --
>>>
>>> 818 char* addr = (char*)regions[i].start();
>>> 819 char* base = os::map_memory(_fd, _full_path, si->_file_offset,
>>> 820 addr, regions[i].byte_size(),
>>> si->_read_only,
>>> 821 si->_allow_exec);
>>>
>>> What happens when the first region succeeds to map but the second
>>> region fails to map? Will both regions be unmapped? I don't see
>>> where you store the return value (base) from os::map_memory(). Does
>>> it mean the code assumes that (addr == base). If so, we need an
>>> assert here.
>>
>> If any of the region fails to map, we bail out and call
>> dealloc_archive_heap_regions(), which handles the deallocation of any
>> regions specified. If second region fails to map, all memory ranges
>> specified by ‘regions’ array are deallocated. We don’t unmap the
>> memory here since it is part of the java heap. Unmapping of heap
>> memory are handled by GC code. The ‘if’ check below makes sure base
>> == addr.
>>
>> if (base == NULL || base != addr) {
>> // dealloc the regions from java heap
>> dealloc_archive_heap_regions(regions, region_num);
>> if (log_is_enabled(Info, cds)) {
>> log_info(cds)("UseSharedSpaces: Unable to map at required
>> address in java heap.");
>> }
>> return false;
>> }
>>
>>
>>>
>>> constantPool.cpp
>>>
>>> Handle refs_handle;
>>> ...
>>> refs_handle = Handle(THREAD, (oop)archived);
>>>
>>> This will first create a NULL handle, then construct a temporary
>>> handle, and then assign the temp handle back to the null
>>> handle. This means two handles will be pushed onto
>>> THREAD->metadata_handles()
>>>
>>> I think it's more efficient if you merge these into a single statement
>>>
>>> Handle refs_handle(THREAD, (oop)archived);
>>
>> Fixed.
>>
>>>
>>> Is this experimental code? Maybe it should be removed?
>>>
>>> 664 if (tag_at(index).is_unresolved_klass()) {
>>> 665 #if 0
>>> 666 CPSlot entry = cp->slot_at(index);
>>> 667 Symbol* name = entry.get_symbol();
>>> 668 Klass* k = SystemDictionary::find(name, NULL, NULL, THREAD);
>>> 669 if (k != NULL) {
>>> 670 klass_at_put(index, k);
>>> 671 }
>>> 672 #endif
>>> 673 } else
>>
>> Removed.
>>
>>>
>>> cpCache.hpp:
>>>
>>> u8 _archived_references
>>>
>>> shouldn't this be declared as an narrowOop to avoid the type casts
>>> when it's used?
>>
>> Ok.
>>
>>>
>>> cpCache.cpp:
>>>
>>> add assert so that one of these is used only at dump time and the
>>> other only at run time?
>>>
>>> 610 oop ConstantPoolCache::archived_references() {
>>> 611 return oopDesc::decode_heap_oop((narrowOop)_archived_references);
>>> 612 }
>>> 613
>>> 614 void ConstantPoolCache::set_archived_references(oop o) {
>>> 615 _archived_references = (u8)oopDesc::encode_heap_oop(o);
>>> 616 }
>>
>> Ok.
>>
>> Thanks!
>>
>> Jiangli
>>
>>>
>>> Thanks!
>>> - Ioi
>>>
>>> On 7/27/17 1:37 PM, Jiangli Zhou wrote:
>>>> Sorry, the mail didn’t handle the rich text well. I fixed the
>>>> format below.
>>>>
>>>> Please help review the changes for JDK-8179302 (Pre-resolve
>>>> constant pool string entries and cache resolved_reference arrays
>>>> in CDS archive). Currently, the CDS archive can contain cached
>>>> class metadata, interned java.lang.String objects. This RFE adds
>>>> the constant pool ‘resolved_references’ arrays (hotspot specific)
>>>> to the archive for startup/runtime performance enhancement.
>>>> The ‘resolved_references' arrays are used to hold references of
>>>> resolved constant pool entries including Strings, mirrors, etc.
>>>> With the 'resolved_references’ being cached, string constants in
>>>> shared classes can now be resolved to existing interned
>>>> java.lang.Strings at CDS dump time. G1 and 64-bit platforms are
>>>> required.
>>>>
>>>> The GC changes in the RFE were discussed and guided by Thomas
>>>> Schatzl and GC team. Part of the changes were contributed by Thomas
>>>> himself.
>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8179302
>>>> hotspot:
>>>> http://cr.openjdk.java.net/~jiangli/8179302/webrev.hotspot.01/
>>>> whitebox: http://cr.openjdk.java.net/~jiangli/8179302/webrev.whitebox.01/
>>>>
>>>> Please see below for details of supporting cached
>>>> ‘resolved_references’ and pre-resolving string constants.
>>>>
>>>> Types of Pinned G1 Heap Regions
>>>>
>>>> The pinned region type is a super type of all archive region types,
>>>> which include the open archive type and the closed archive type.
>>>>
>>>> 00100 0 [ 8] Pinned Mask
>>>> 01000 0 [16] Old Mask
>>>> 10000 0 [32] Archive Mask
>>>> 11100 0 [56] Open Archive: ArchiveMask | PinnedMask | OldMask
>>>> 11100 1 [57] Closed Archive: ArchiveMask | PinnedMask | OldMask + 1
>>>>
>>>>
>>>> Pinned Regions
>>>>
>>>> Objects within the region are 'pinned', which means GC does not
>>>> move any live objects. GC scans and marks objects in the
>>>> pinned region as normal, but skips forwarding live objects.
>>>> Pointers in live objects are updated. Dead objects (unreachable)
>>>> can be collected and freed.
>>>>
>>>> Archive Regions
>>>>
>>>> The archive types are sub-types of 'pinned'. There are two types of
>>>> archive region currently, open archive and closed archive. Both can
>>>> support caching java heap objects via the CDS archive.
>>>>
>>>> An archive region is also an old region by design.
>>>>
>>>> Open Archive (GC-RW) Regions
>>>>
>>>> Open archive region is GC writable. GC scans & marks objects within
>>>> the region and adjusts (updates) pointers in live objects the same
>>>> way as a pinned region. Live objects (reachable) are pinned and not
>>>> forwarded by GC.
>>>> Open archive region does not have 'dead' objects. Unreachable
>>>> objects are 'dormant' objects. Dormant objects are not
>>>> collected and freed by GC.
>>>>
>>>> Adjustable Outgoing Pointers
>>>>
>>>> As GC can adjust pointers within the live objects in open archive
>>>> heap region, objects can have outgoing pointers to another
>>>> java heap region, including closed archive region, open archive
>>>> region, pinned (or humongous) region, and normal generational
>>>> region. When a referenced object is moved by GC, the pointer within
>>>> the open archive region is updated accordingly.
>>>>
>>>> Closed Archive (GC-RO) Regions
>>>>
>>>> The closed archive region is GC read-only region. GC cannot write
>>>> into the region. Objects are not scanned and marked by GC. Objects
>>>> are pinned and not forwarded. Pointers are not updated by GC
>>>> either. Hence, objects within the archive region cannot have any
>>>> outgoing pointers to another java heap region. Objects however can
>>>> still have pointers to other objects within the closed archive
>>>> regions (we might allow pointers to open archive regions in the
>>>> future). That restricts the type of java objects that can
>>>> be supported by the archive region.
>>>> In JDK 9 we support archive Strings with the archive regions.
>>>>
>>>> The GC-readonly archive region makes java heap memory sharable
>>>> among different JVM processes. NOTE: synchronization on the objects
>>>> within the archive heap region can still cause writes to the memory
>>>> page.
>>>>
>>>> Dormant Objects
>>>>
>>>> Dormant objects are unreachable java objects within the open
>>>> archive heap region.
>>>> A java object in the open archive heap region is a live object if
>>>> it can be reached during scanning. Some of the java objects in
>>>> the region may not be reachable during scanning. Those objects are
>>>> considered as dormant, but not dead. For example, a constant pool
>>>> 'resolved_references' array is reachable via the klass root if its
>>>> container klass (shared) is already loaded at the time during GC
>>>> scanning. If a shared klass is not yet loaded, the klass root is
>>>> not scanned and it's constant pool 'resolved_reference' array
>>>> (A) in the open archive region is not reachable. Then A is a
>>>> dormant object.
>>>>
>>>> Object State Transition
>>>>
>>>> All java objects are initially dormant objects when open archive
>>>> heap regions are mapped to the runtime java heap. A dormant object
>>>> becomes live object when the associated shared class is loaded at
>>>> runtime. Explicit call to G1SATBCardTableModRefBS::enqueue() needs
>>>> to be made when a dormant object becomes live. That should be the
>>>> case for cached objects with strong roots as well, since strong
>>>> roots are only scanned at the start of GC marking (the initial
>>>> marking) but not during Remarking/Final marking. If a cached object
>>>> becomes live during concurrent marking phase, G1 may not find it
>>>> and mark it live unless a call to
>>>> G1SATBCardTableModRefBS::enqueue() is made for the object.
>>>>
>>>> Currently, a live object in the open archive heap region cannot
>>>> become dormant again. This restriction simplifies GC
>>>> requirement and guarantees all outgoing pointers are updated by GC
>>>> correctly. Only objects for shared classes from the builtin class
>>>> loaders (boot, PlatformClassLoaders, and AppClassLoaders) are
>>>> supported for caching.
>>>>
>>>> Caching Java Objects at Archive Dump Time
>>>>
>>>> The closed archive and open archive regions are allocated near the
>>>> top of the dump time java heap. Archived java objects are copied
>>>> into the designated archive heap regions. For example, String
>>>> objects and the underlying 'value' arrays are copied into
>>>> the closed archive regions. All references to the archived objects
>>>> (from shared class metadata, string table, etc) are set to the
>>>> new heap locations. A hash table is used to keep track of all
>>>> archived java objects during the copying process to make sure java
>>>> object is not archived more than once if reached from different
>>>> roots. It also makes sure references to the same archived object
>>>> are updated using the same new address location.
>>>>
>>>> Caching Constant Pool resolved_references Array
>>>>
>>>> The 'resolved_references' is an array that holds references of
>>>> resolved constant pool entries including Strings, mirrors
>>>> and methodTypes, etc. Each loaded class has one
>>>> 'resolved_references' array (in ConstantPoolCache). The
>>>> 'resolved_references' arrays are copied into the open archive
>>>> regions during dump process. Prior to copying the
>>>> 'resolved_references' arrays, JVM iterates through constant pool
>>>> entries and resolves all JVM_CONSTANT_String entries to existing
>>>> interned Strings for all archived classes. When resolving, JVM only
>>>> looks up the string table and finds existing interned Strings
>>>> without inserting new ones. If a string entry cannot be resolved to
>>>> an existing interned String, the constant pool entry remain as
>>>> unresolved. That prevents memory waste if a constant pool string
>>>> entry is never used at runtime.
>>>>
>>>> All String objects referenced by the string table are copied first
>>>> into the closed archive regions. The string table entry is
>>>> updated with the new location when each String object is archived.
>>>> The JVM updates the resolved constant pool string entries with the
>>>> new object locations when copying the 'resolved_references' arrays
>>>> to the open archive regions. References to
>>>> the 'resolved_references' arrays in the ConstantPoolCache are also
>>>> updated.
>>>> At runtime as part of ConstantPool::restore_unshareable_info()
>>>> work, call G1SATBCardTableModRefBS::enqueue() to let GC know the
>>>> 'resolved_references' is becoming live. A handle is created for the
>>>> cached object and added to the loader_data's handles.
>>>>
>>>> Runtime Java Heap With Cached Java Objects
>>>>
>>>>
>>>> The closed archive regions (the string regions) and open archive
>>>> regions are mapped to the runtime java heap at the same offsets as
>>>> the dump time offsets from the runtime java heap base.
>>>>
>>>> Preliminary test execution and status:
>>>>
>>>> JPRT: passed
>>>> Tier2-rt: passed
>>>> Tier2-gc: passed
>>>> Tier2-comp: passed
>>>> Tier3-rt: passed
>>>> Tier3-gc: passed
>>>> Tier3-comp: passed
>>>> Tier4-rt: passed
>>>> Tier4-gc: passed
>>>> Tier4-comp:6 jobs timed out, all other tests passed
>>>> Tier5-rt: one test failed but passed when running locally, all
>>>> other tests passed
>>>> Tier5-gc: passed
>>>> Tier5-comp: running
>>>> hotspot_gc: two jobs timed out, all other tests passed
>>>> hotspot_gc in CDS mode: two jobs timed out, all other tests passed
>>>> vm.gc: passed
>>>> vm.gc in CDS mode: passed
>>>> Kichensink: passed
>>>> Kichensink in CDS mode: passed
>>>>
>>>> Thanks,
>>>> Jiangli
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20170804/94de9413/attachment.htm>
More information about the hotspot-gc-dev
mailing list