RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address

[Ioi Lam]

On 10/16/19 8:49 PM, Volker Simonis wrote:
> On Thu, Oct 17, 2019 at 1:58 AM Jiangli Zhou <jianglizhou at google.com> 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.
>>
> Hi Jiangli,
>
> happy to see that you are still working on this and continuously improve CDS :)
>
> I currently only have a quick question: if you relocate the contents
> of the CDS archive, does this mean that you have to map the whole
> archive r/w and basically modify all the pages? In that case, CDS
> would only help to improve startup time but not for reducing the
> memory footprint (i.e. RSS) of multiple JVM running in parallel using
> the same archive. Is that correct?
Yes, that's correct.

One possibility is to reorder the data to segregate the objects that 
have no embedded pointers. This could allow perhaps 20~30% of the 
archive to be shared, even with pointer patching.

Also, if we follow up with the incremental patching idea proposed by 
Jiangli, any classes that are not yet loaded will not be modified.

In our testing environment, we have a fairly small number of mapping 
failures (below 1%), so I don't think the performance degradation caused 
by relocation matters too much. However, now that we can guarantee that 
CDS is always enabled, we can do more optimizations. E.g., AOT-compiled 
code can now have references into the CDS data and can assume that it's 
always available. So hopefully CDS can be used as a basic building block 
for future optimization work.


Thanks
- Ioi

>
> Best regards,
> Volker
>
>> 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);
>> +  }
>> +}
>> +
>>
>> How about renaming update_archived_mirror_native_pointers to
>> update_archived_mirror_klass_pointers.
>>
>> It would be good to pass the current klass as an argument. We can
>> verify the relocated pointer matches with the current klass pointer.
>>
>> We should also check if relocation is necessary before spending cycles
>> to obtain the klass pointer from the mirror.
>>
>> 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?
>>
>> - 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?
>>
>> - 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.
>>
>> Will send out the rest of the review comments later.
>>
>> 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
>>>