RFR (L) 8072061: Automatically determine optimal sizes for the CDS regions

[Jiangli Zhou]

Hi Ioi,

Sorry for the delay. Below are my feedbacks/questions. They are mostly minor issues. One question that’s not listed below is for metaspace_pointers_do() in various classes. If a new metadata pointer field is added in those classes, metaspace_pointers_do() needs to be also updated. To help others be aware of that, it might be useful to add comments in those classes to indicate that. Also, it might be a good idea to run the IsRefInArchiveChecker for non-debug build. How slow is the check?

src/share/vm/memory/metaspaceShared.cpp

- Line 77, od should stand for optional data. I also have a question. We had planned to make the od region optionally mapped. Would that still be doable with the change?

- Line 148, can you please change it into multiple lines following our coding convention.
148     if (total == 0) {total = 1;}
- Line 201, with MetaspaceShared::initialize_shared_rs() change, does it affect the java heap location? Could you please outline how class space, shared space and heap are arranged now? It would be helpful to describe that in the comment above the function.

- Line 271, MetaspaceShared::commit_shared_space_to() could fail. However, the failure cases are not handled and the caller sets the new _top unconditionally. MetaspaceShared::commit_shared_space_to() should return boolean to indicate the memory are committed or not. The failure cases should be handled properly.

- Line 823, should the assert be uncommented?

- Line 877, could you please move ArchiveCompactor class to it’s own file?

- Line 891, how about rename MyTable to RelocationCache?

- Line 909 & 911, can allocation in RO/RW region fail? If so, we need to handle the failure case.

- Line 1110, in rare case the string space top may not be equal to ‘base+shared_string_bytes’ if the two MemRegions are not consecutive. The DumpRegion::init() could take a ‘size’ argument. Or, DumpRegion should not be used for string space, as most of work in DumpRegion does not apply to string space. For example, _st_region.pack() at line 1112 does not seem to be necessary.

- Line 1193 & 1194, it seems classes are relocated after string copying (which also updates the klass pointer within the objects)? Or, classes are already copied to the new location before string objects are handled, and ArchiveCompactor::relocate_well_known_klass() just updates the class pointers in SystemDictionary::_well_known_klasses? If it’s the latter, maybe rename ArchiveCompactor::relocate_well_known_klass() to ArchiveCompactor::update_well_known_klass() to avoid possible confusion.

src/share/vm/classfile/symbolTable.cpp

- Line 84, could you please add a comment explain the change?

src/share/vm/classfile/dictionary.cpp

- Dictionary::classes_do(), the comment indicates the function is used for dump time only. Could you please replace the 'if (DumpSharedSpaces)’ check with an assert in that case?

src/os_cpu/windows_x86/vm/thread_windows_x86.cpp
src/share/vm/classfile/sharedClassUtil.hpp
src/share/vm/memory/filemap.hpp
src/share/vm/memory/virtualspace.hpp
src/share/vm/oops/annotations.cpp
src/share/vm/oops/constMethod.cpp
src/share/vm/oops/constMethod.hpp
src/share/vm/oops/methodCounters.hpp
test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java
- Copyright header needs to be updated.

Thanks,
Jiangli

> On Jun 29, 2017, at 3:55 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8072061
> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/
> 
> ------------------------------------------------------------------------------
> Motivation:
> 
>  The CDS archive is divided in several regions such as RO, RW, etc. In
>  JDK 9, when you are dumping with a custom classlist, you must manually
>  specify the region sizes using flags such as -XX:SharedReadOnlySize.
>  This is error prone and cumbersome.
> 
>  This RFE makes it possible to automatically determine the optimal sizes
>  of the regions.
> 
> Overview:
> 
>  With this RFE, during CDS dump time, all class metadata are first
>  allocated outside of the shared regions. Then, we scan all of the
>  reachable metadata objects, determine whether they are RO or RW, and
>  then copy them into the destination regions.
> 
>  After all objects are copied, we scan and relocate all the pointers to
>  point to the new copies.
> 
> Notes to reviewers:
> 
>  To implement the copy and relocation operations, we have added the new
>  MetaspaceClosure API for walking all the reachablemetadata objects, and
>  scanning all the pointers embedded in these objects.
> 
>  Please start in metaspaceShared.cppand follow the code path:
> 
>    ArchiveCompactor::copy_and_compact()
>    -> iterate_roots()
>      -> SystemDictionary::classes_do(it)
>        -> Dictionary::classes_do(MetaspaceClosure* it)
> -> it->push(probe->klass_addr())
> 
>  At this point you will get into metaspaceClosure.hpp. Please read the
>  comments around MetaspaceClosure::Ref about the (lack of) C++ vtables,
>  and why the template classes ObjectRef, PrimitiveArrayRef and
>  PointerArrayRef are needed.
> 
>  Once you understand how ObjectRefand PointerArrayRef work, you can
>  see that the above it->push(probe->klass_addr()) call will be invoking
>  this template method:
> 
>     template <class T> void push(T** mpp, Writability w = _default) {
>       ObjectRef<T> ref(mpp);
>       push_impl(&ref, w);
>     }
> 
>  which will eventually call into Klass::metaspace_pointers_do(), which
>  will eventually recursively iterate over all the Klasses, Methods,
>  ConstantPools and other types of MetaspaceObj.
> 
> Testingstatus:
> 
>  All CDS tests have passed locally on Linux. All new tests have been
> implemented.
> 
> I am waiting to run RBT tests on other platforms.
> 
> Thanks
> - Ioi