RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Calvin Cheung
calvin.cheung at oracle.com
Tue Oct 22 15:57:01 UTC 2019
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?
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?
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");
metaspaceShared.cpp
Extra blank line at 1223.
Blank line 1353 was removed.
saproc.cpp
667 jlong baseAddress = (mapping_offset == 0) ? 0 :
(mapping_offset + (jlong)sharedBaseAddress);
Why is baseAddress set to 0 if mapping_offset is 0?
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