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

[Jiangli Zhou]

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.

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
>