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

[Volker Simonis]

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?

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