RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Ioi Lam
ioi.lam at oracle.com
Tue Oct 29 03:57:30 UTC 2019
Hi Calvin,
Thanks for the review. I've updated the patch according to your
comments. Here's the delta from the last webrev:
http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v03-delta/
(I'll create a full webrev after integrating Jiangli's latest comments).
Please see my replies in-line:
On 10/22/19 8:57 AM, Calvin Cheung wrote:
> Hi Ioi,
>
> The changes look good overall.
>
> Below are my initial comments. I may have more comments later.
>
> thanks,
>
> Calvin
>
> dynamicArchive.cpp
>
> 140 //tty->print_cr("%p", a_name);
>
> 144 //tty->print_cr("%p", b_name);
>
> Leftover debug statements?
>
I have removed these.
> filemap.hpp
>
> 292 void set_shared_path_table(size_t offset, int size) {
> 293 _shared_path_table_offset = offset;
> 294 _shared_path_table_size = size;
> 295 }
>
> Unused function?
>
Removed.
> filemap.cpp
>
> 1464 if (!rs.is_reserved()) {
> 1465 // When mapping on Windows with (addr_delta == 0), we don't
> reserve the address space for the regions
> 1466 // (Windows can't mmap into a ReservedSpace). In this case,
> NMT requires we call it after
> 1467 // os::map_memory has succeeded.
>
> Maybe add an assert like the following?
>
> assert(MetaspaceShared::use_windows_memory_mapping(), "Windows
> memory mapping only");
Added.
>
> metaspaceShared.cpp
>
> Extra blank line at 1223.
> Blank line 1353 was removed.
>
I reverted those changes.
> saproc.cpp
>
> 667 jlong baseAddress = (mapping_offset == 0) ? 0 :
> (mapping_offset + (jlong)sharedBaseAddress);
>
> Why is baseAddress set to 0 if mapping_offset is 0?
The intention was to avoid using the newly added bitmap region, which
has a mapping_offset of 0. Anyway, this is pretty hacky, so I added a
new field CDSFileMapRegion::_is_bitmap_region instead.
Thanks
- Ioi
>
> On 10/16/19 7:43 PM, Ioi Lam wrote:
>> Hi Jiangli,
>>
>> Thanks for the review. Here's a delta from the previous webrev
>> according to your suggestions:
>>
>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v02-delta/
>>
>>
>> (I also fixed error handling code -- the
>> SharedArchiveConsistency.java test was failing when
>> -vmoption:-XX:SharedBaseAddress=0 was passed to jtreg).
>>
>> On 10/16/19 4:58 PM, Jiangli Zhou wrote:
>>> Hi Ioi,
>>>
>>> This is another great step for CDS usability improvement. Thank you!
>>>
>>> I have a high level question (or request): could we consider
>>> separating the relocation work for 'direct' class metadata from other
>>> types of metadata (such as the shared system dictionary, symbol table,
>>> etc)? Initially we only relocate the tables and other archived global
>>> data. When each archived class is being loaded, we can relocate all
>>> the pointers within the current class. We could find the segment (for
>>> the current class) in the bitmap and update the pointers within the
>>> segment. That way we can reduce initial startup costs and also avoid
>>> relocating class data that's not used at runtime. In some real world
>>> large systems, an archive may contain extremely large number of
>>> classes.
>>
>> I think that's a very good idea. For this RFE, I'd like to limit the
>> amount of change (it's already a lot). Maybe we can do that as a
>> follow-up RFE.
>>
>> To do what you described, when we find an InstanceKlass K from the
>> shared dictionary, we just scan all pointers starting from K. Each
>> time we see a pointer, we check if it is marked in the bitmap. If so,
>> we unmask the bit, update the pointer, and recursively scan the
>> object referenced by this pointer.
>>
>> I think this will be a good trade off for lower star-up pause. The
>> cost is more work done per loaded classes, and we also need to keep
>> the bitmap in R/W memory during the entire lifetime of the JVM.
>>
>>> Following are partial review comments so we can move things forward.
>>> Still going through the rest of the changes.
>>>
>>> - src/hotspot/share/classfile/javaClasses.cpp
>>>
>>> 1218 void java_lang_Class::update_archived_mirror_native_pointers(oop
>>> archived_mirror) {
>>> 1219 Klass* k =
>>> ((Klass*)archived_mirror->metadata_field(_klass_offset));
>>> 1220 if (k != NULL) { // k is NULL for the primitive classes such as
>>> java.lang.Byte::TYPE <<<<<<<<<<<
>>> 1221 archived_mirror->metadata_field_put(_klass_offset,
>>> (Klass*)(address(k) + MetaspaceShared::mapping_delta()));
>>> 1222 }
>>> 1223 ...
>>>
>>> Primitive type mirrors are handled separately. Could you please verify
>>> if this call path happens for primitive type mirror?
>>>
>>> To answer my question above, looks like you added the following, which
>>> is to be used for primitive type mirrors. That seems to be the reason
>>> why update_archived_mirror_native_pointers is trying to also cover
>>> primitive type. It better to have a separate API for primitive type
>>> mirror, which is cleaner. And, we also can replace the above check at
>>> line 1220 to be an assert for regular mirrors.
>>>
>>> +void ReadClosure::do_mirror_oop(oop *p) {
>>> + do_oop(p);
>>> + oop mirror = *p;
>>> + if (mirror != NULL) {
>>> + java_lang_Class::update_archived_mirror_native_pointers(mirror);
>>> + }
>>> +}
>>> +
>>
>> I added a separate update_archived_primitive_mirror_native_pointers()
>> function as you suggested, and modified universe.cpp to call this
>> function explicitly. I removed ReadClousure::do_mirror_oop().
>>
>> I also added a macro in universe.cpp to avoid repeating the same code
>> for the 9 mirrors.
>>
>>> How about renaming update_archived_mirror_native_pointers to
>>> update_archived_mirror_klass_pointers.
>>
>> It updates any native pointers that are in mirrors. It just turns out
>> that those pointers are all Klass pointers at the moment.
>>
>> Also "klass_pointer" could be incorrectly interpreted to mean the
>> klass stored in the object's header word.
>>
>>> It would be good to pass the current klass as an argument. We can
>>> verify the relocated pointer matches with the current klass pointer.
>>
>> I added an assert like this:
>>
>> update_archived_mirror_native_pointers(m);
>> assert(as_Klass(m) == k, "must be");
>>
>>> We should also check if relocation is necessary before spending cycles
>>> to obtain the klass pointer from the mirror.
>>
>> I've added a check for (MetaspaceShared::mapping_delta() != 0) in
>> update_archived_mirror_native_pointers().
>>
>>
>>>
>>> 1252 update_archived_mirror_native_pointers(m);
>>> 1253
>>> 1254 // mirror is archived, restore
>>> 1255 assert(HeapShared::is_archived_object(m), "must be archived
>>> mirror object");
>>> 1256 Handle mirror(THREAD, m);
>>>
>>> Could we move the line at 1252 after the assert at line 1255?
>>
>> Done
>>
>>> - src/hotspot/share/include/cds.h
>>>
>>> 47 int _mapped_from_file; // Is this region mapped from a
>>> file?
>>> 48 // If false, this region was
>>> initialized using os::read().
>>>
>>> Is the new field truly needed? It seems we could use _mapped_base to
>>> determine if a region is mapped or not?
>>
>> Yes, it's needed for Windows. We set "_mapped_base" even when the
>> region is not actually mapped using os::map_memory. See
>> FileMapInfo::map_region.
>>
>>> - src/hotspot/share/memory/dynamicArchive.cpp
>>>
>>> Could you please remove the debugging print code in
>>> dynamic_dump_method_comparator? Or convert those to logging output if
>>> they are helpful.
>>
>> I removed them.
>>
>>> Will send out the rest of the review comments later.
>>
>> Thanks
>> - Ioi
>>
>>>
>>> Best,
>>>
>>> Jiangli
>>>
>>>
>>>
>>>
>>> On Thu, Oct 10, 2019 at 6:00 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8231610
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v01/
>>>>
>>>>
>>>> Design:
>>>> http://cr.openjdk.java.net/~iklam/jdk14/design/8231610-relocate-cds-archive.txt
>>>>
>>>>
>>>>
>>>> Overview:
>>>>
>>>> The CDS archive is mmaped to a fixed address range (starting at
>>>> SharedBaseAddress, usually 0x800000000). Previously, if this
>>>> requested address range is not available (usually due to Address
>>>> Space Layout Randomization (ASLR) [2]), the JVM will give up and
>>>> will load classes dynamically using class files.
>>>>
>>>> [a] This causes slow down in JVM start-up.
>>>> [b] Handling of mapping failures causes unnecessary complication in
>>>> the CDS tests.
>>>>
>>>> Here are some preliminary benchmarking results (using default CDS
>>>> archive,
>>>> running helloworld):
>>>>
>>>> (a) 47.1ms (CDS enabled, mapped at requested addr)
>>>> (b) 53.8ms (CDS enabled, mapped at alternate addr)
>>>> (c) 86.2ms (CDS disabled)
>>>>
>>>> The small degradation in (b) is caused by the relocation of
>>>> absolute pointers embedded in the CDS archive. However, it is
>>>> still a big improvement over case (c)
>>>>
>>>> Please see the design doc (link above) for details.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>
More information about the hotspot-runtime-dev
mailing list