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

[Ioi Lam]

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
>>