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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore

Ioi, I have one comment and one question.

http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/hotspot/src/share/vm/memory/metaspace.cpp.udiff.html

This cleanup is worth all of the change.   It's nice to not conflate
metaspace allocation with the creating shared region anymore.   It
started out as a little bit of extra code but has expanded to all of
what you removed.   So I guess you've convinced me that copying metadata
is worth doing.

http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/hotspot/src/share/vm/utilities/hashtable.cpp.udiff.html

Why do you have to include metaspaceShared.hpp here?

I've prereviewed this and have no further comments.   This looks good!  
Thank you for making the changes I suggested in the pre-review.  
Starting with copy_and_compact() it makes sense what this is doing and
looks like it'll be easier to understand overall.

Thanks!
Coleen


On 6/29/17 6:55 PM, Ioi Lam 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
Hi Colleen,

Thanks for the review and all of your help to fix my previous
implementations, which were a bit hacky to say the least :-)


On 6/30/17 3:16 PM, [hidden email] wrote:

>
> Ioi, I have one comment and one question.
>
> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/hotspot/src/share/vm/memory/metaspace.cpp.udiff.html 
>
>
> This cleanup is worth all of the change.   It's nice to not conflate
> metaspace allocation with the creating shared region anymore.   It
> started out as a little bit of extra code but has expanded to all of
> what you removed.   So I guess you've convinced me that copying
> metadata is worth doing.
>
> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/hotspot/src/share/vm/utilities/hashtable.cpp.udiff.html 
>
>
> Why do you have to include metaspaceShared.hpp here?
>
Oops, that was added by accident. I removed it.

Thanks
- Ioi

> I've prereviewed this and have no further comments. This looks good!  
> Thank you for making the changes I suggested in the pre-review.  
> Starting with copy_and_compact() it makes sense what this is doing and
> looks like it'll be easier to understand overall.
>
> Thanks!
> Coleen
>
>
> On 6/29/17 6:55 PM, Ioi Lam 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Mikhailo Seledtsov
In reply to this post by Ioi Lam
Hi Ioi,

   Changes to test code look good.


Misha


On 06/29/2017 03:55 PM, Ioi Lam 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
Thanks Misha!

- Ioi


On 7/5/17 3:49 PM, mikhailo wrote:

> Hi Ioi,
>
>   Changes to test code look good.
>
>
> Misha
>
>
> On 06/29/2017 03:55 PM, Ioi Lam 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jiangli Zhou
In reply to this post by Ioi Lam
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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
Hi Jiangli,

Thanks for the very detailed review! Please see my responses in-line

I have uploaded a delta from the previous webrev at

http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v11.delta/

On 7/26/17 2:59 PM, Jiangli Zhou wrote:
> 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.

Can you suggest a place for adding such comments?
<Class>::metaspace_pointers_do() doesn't seem to be the right place --
if you're reading this function, you should already know what you're
supposed to do.

I think it's fairly easy to crash in the CDS tests if you missed a field
-- the JVM would dereference an invalid pointer. However, when such
crash happens, I don't know what's the best way to tell the developer
that a field might be missed in metaspace_pointers_do().

> Also, it might be a good idea to run the IsRefInArchiveChecker for
> non-debug build. How slow is the check?

It's not too bad, but it doesn't help with the case of missing fields.
We just check that for all fields that you have remembered to scan, they
point to good addresses. This check is for the implementation of
ArchiveCompactor, not for the completeness of the
metaspace_pointers_do() functions.

>
> src/share/vm/memory/metaspaceShared.cpp
>
> - Line 77, od should stand for optional data.

Fixed.

> I also have a question. We had planned to make the od region
> optionally mapped. Would that still be doable with the change?
>

Yes. This change only moves the location of the OD section and doesn't
affect it in any other ways.

> - Line 148, can you please change it into multiple lines following our
> coding convention.
> 148 if (total == 0) {total = 1;}

Fixed.

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

How about this comment?

/*
  * On 64-bit VM, the heap and class space layout will be the same as if
  * you're running in -Xshare:on mode:
  *
  *                         +-- SharedBaseAddress (default = 0x800000000)
  *                         v
  * +-..---------+----+ ... +----+----+----+----+----+---------------+
  * |    Heap    | ST |     | MC | RW | RO | MD | OD | class space   |
  * +-..---------+----+ ... +----+----+----+----+----+---------------+
  * |<--MaxHeapSize->|     |<-- UnscaledClassSpaceMax = 4GB ------->|
  *
  */

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

I changed it to vm_exit_during_initialization, as there isn't much we
can when you completely run out of memory in the current process.

> - Line 823, should the assert be uncommented?
>

Fixed. There was a bug in calculating the RO sizes, but I fixed that
too. See ArchiveCompactor::allocate() in the webrev.

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

I agree that metaspaceShared.cpp is getting too big. However, I've made
a lot of changes in this file already, and I want to keep track of the
changes in the repo's history. If I move ArchiveCompactor now, I need to
change other parts of metaspaceShared.cpp, and there will be confusion
about what change is for implementing this RFE, and what changes are
related to the cleanup.

I think it's better to do the cleanup of metaspaceShared.cpp in a
separate RFE, so we just change one thing at a time.

> - Line 891, how about rename MyTable to RelocationCache?
>

Fixed. I changed it to RelocationTable since it's a type of
ResourceHashtable.

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

It will never fail. The only failure is failure to commit memory, which
will cause VM to exit.

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

I changed the calculation of the string regions so now the output looks
like this for a small number of strings:


mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4%
used] at 0x0000000800000000
rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9%
used] at 0x0000000800006000
ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0%
used] at 0x000000080040a000
md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4%
used] at 0x0000000800adf000
od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9%
used] at 0x0000000800ae1000
s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0%
used] at 0x00000007bfc00000
s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0%
used] at 0x0000000000000000
total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9% used]


and this for a large number of strings. (I added a new stress test case,
but unfortunately that needed to be a closed test.).


mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4%
used] at 0x0000000800000000
rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9%
used] at 0x0000000800006000
ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0%
used] at 0x000000080040a000
md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4%
used] at 0x0000000800adf000
od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9%
used] at 0x0000000800ae1000
s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0%
used] at 0x00000007bfc00000
s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0%
used] at 0x0000000000000000
total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9% used]

The calculation is done inside FileMapInfo::write_string_regions, so I
don't have to duplicated the logic else where. I keep the use of
DumpRegion for _s0_region and _s1_region so that the size statistics
printing code can be uniform.

By the way, I found the logic in FileMapInfo::write_string_regions a
little hard to understand, so I added these comments:

// Here's the mapping from (GrowableArray<MemRegion> *regions) ->
(metaspace string regions).
//   + We have 1 or more heap regions: r0, r1, r2 ..... rn
//   + We have 2 metaspace string regions: s0 and s1
//
// If there's a single heap region (r0), then s0 == r0, and s1 is empty.
// Otherwise:
//
// "X" represented space that's occupied by heap objects.
// "_" represented unused spaced in the heap region.
//
//
//    |r0        | r1  | r2 | ...... | rn |
//    |XXXXXX|__ |XXXXX|XXXX|XXXXXXXX|XXXX|
//    |<-s0->|   |<- s1 ----------------->|
//            ^^^
//             |
//             +-- unmapped space

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

"Relocate" means "update a pointer to point to the new location of a
copied object". Here the action is

       relocate (the pointers in) SystemDictionary::_well_known_klasses[]

I think "update" is too generic. The use of 'relocate' is consistent
with the rest of the operations

     {
       tty->print_cr("Relocating embedded pointers ... ");
       ResourceMark rm;
       ShallowCopyEmbeddedRefRelocator emb_reloc;
       iterate_roots(&emb_reloc);
     }
     {
       tty->print_cr("Relocating external roots ... ");
       ResourceMark rm;
       RefRelocator ext_reloc;
       iterate_roots(&ext_reloc);
     }


> src/share/vm/classfile/symbolTable.cpp
>
> - Line 84, could you please add a comment explain the change?
>

I reverted the change. During dump time, the _shared_table is empty so
this is a safe no-op.

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

Fixed.

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

Fixed

Thanks again!

- Ioi

>
> Thanks,
> Jiangli
>
>> On Jun 29, 2017, at 3:55 PM, Ioi Lam <[hidden email]
>> <mailto:[hidden email]>> 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Jiangli Zhou
Hi Ioi,

Thank you so much for making the additional changes. I’d suggest to add the ‘warning’ comment above the first metadata field in each class containing metaspace_pointers_do(). No need for new webrev for the comment changes.

Thanks!

Jiangli

> On Jul 27, 2017, at 12:24 PM, Ioi Lam <[hidden email]> wrote:
>
> Hi Jiangli,
>
> Thanks for the very detailed review! Please see my responses in-line
>
> I have uploaded a delta from the previous webrev at
>
> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v11.delta/ <http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v11.delta/>
>
> On 7/26/17 2:59 PM, Jiangli Zhou wrote:
>> 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.
>
> Can you suggest a place for adding such comments? <Class>::metaspace_pointers_do() doesn't seem to be the right place -- if you're reading this function, you should already know what you're supposed to do.
>
> I think it's fairly easy to crash in the CDS tests if you missed a field -- the JVM would dereference an invalid pointer. However, when such crash happens, I don't know what's the best way to tell the developer that a field might be missed in metaspace_pointers_do().
>
>> Also, it might be a good idea to run the IsRefInArchiveChecker for non-debug build. How slow is the check?
>
> It's not too bad, but it doesn't help with the case of missing fields. We just check that for all fields that you have remembered to scan, they point to good addresses. This check is for the implementation of ArchiveCompactor, not for the completeness of the metaspace_pointers_do() functions.
>
>>
>> src/share/vm/memory/metaspaceShared.cpp
>>
>> - Line 77, od should stand for optional data.
>
> Fixed.
>
>> I also have a question. We had planned to make the od region optionally mapped. Would that still be doable with the change?
>>
>
> Yes. This change only moves the location of the OD section and doesn't affect it in any other ways.
>
>> - Line 148, can you please change it into multiple lines following our coding convention.
>> 148     if (total == 0) {total = 1;}
>
> Fixed.
>
>> - 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.
>>
>
> How about this comment?
>
> /*
>  * On 64-bit VM, the heap and class space layout will be the same as if
>  * you're running in -Xshare:on mode:
>  *
>  *                         +-- SharedBaseAddress (default = 0x800000000)
>  *                         v
>  * +-..---------+----+ ... +----+----+----+----+----+---------------+
>  * |    Heap    | ST |     | MC | RW | RO | MD | OD | class space   |
>  * +-..---------+----+ ... +----+----+----+----+----+---------------+
>  * |<--MaxHeapSize-> |     |<-- UnscaledClassSpaceMax = 4GB ------->|
>  *
>  */
>
>> - 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.
>>
>
> I changed it to vm_exit_during_initialization, as there isn't much we can when you completely run out of memory in the current process.
>
>> - Line 823, should the assert be uncommented?
>>
>
> Fixed. There was a bug in calculating the RO sizes, but I fixed that too. See ArchiveCompactor::allocate() in the webrev.
>
>> - Line 877, could you please move ArchiveCompactor class to it’s own file?
>>
>
> I agree that metaspaceShared.cpp is getting too big. However, I've made a lot of changes in this file already, and I want to keep track of the changes in the repo's history. If I move ArchiveCompactor now, I need to change other parts of metaspaceShared.cpp, and there will be confusion about what change is for implementing this RFE, and what changes are related to the cleanup.
>
> I think it's better to do the cleanup of metaspaceShared.cpp in a separate RFE, so we just change one thing at a time.
>
>> - Line 891, how about rename MyTable to RelocationCache?
>>
>
> Fixed. I changed it to RelocationTable since it's a type of ResourceHashtable.
>
>> - Line 909 & 911, can allocation in RO/RW region fail? If so, we need to handle the failure case.
>>
>
> It will never fail. The only failure is failure to commit memory, which will cause VM to exit.
>
>> - 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.
>>
>
> I changed the calculation of the string regions so now the output looks like this for a small number of strings:
>
>
> mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4% used] at 0x0000000800000000
> rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9% used] at 0x0000000800006000
> ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0% used] at 0x000000080040a000
> md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4% used] at 0x0000000800adf000
> od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9% used] at 0x0000000800ae1000
> s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0% used] at 0x00000007bfc00000
> s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0% used] at 0x0000000000000000
> total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9% used]
>
>
> and this for a large number of strings. (I added a new stress test case, but unfortunately that needed to be a closed test.).
>
>
> mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4% used] at 0x0000000800000000
> rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9% used] at 0x0000000800006000
> ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0% used] at 0x000000080040a000
> md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4% used] at 0x0000000800adf000
> od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9% used] at 0x0000000800ae1000
> s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0% used] at 0x00000007bfc00000
> s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0% used] at 0x0000000000000000
> total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9% used]
>
> The calculation is done inside FileMapInfo::write_string_regions, so I don't have to duplicated the logic else where. I keep the use of DumpRegion for _s0_region and _s1_region so that the size statistics printing code can be uniform.
>
> By the way, I found the logic in FileMapInfo::write_string_regions a little hard to understand, so I added these comments:
>
> // Here's the mapping from (GrowableArray<MemRegion> *regions) -> (metaspace string regions).
> //   + We have 1 or more heap regions: r0, r1, r2 ..... rn
> //   + We have 2 metaspace string regions: s0 and s1
> //
> // If there's a single heap region (r0), then s0 == r0, and s1 is empty.
> // Otherwise:
> //
> // "X" represented space that's occupied by heap objects.
> // "_" represented unused spaced in the heap region.
> //
> //
> //    |r0        | r1  | r2 | ...... | rn |
> //    |XXXXXX|__ |XXXXX|XXXX|XXXXXXXX|XXXX|
> //    |<-s0->|   |<- s1 ----------------->|
> //            ^^^
> //             |
> //             +-- unmapped space
>
>> - 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.
>>
>
> "Relocate" means "update a pointer to point to the new location of a copied object". Here the action is
>
>       relocate (the pointers in) SystemDictionary::_well_known_klasses[]
>
> I think "update" is too generic. The use of 'relocate' is consistent with the rest of the operations
>
>     {
>       tty->print_cr("Relocating embedded pointers ... ");
>       ResourceMark rm;
>       ShallowCopyEmbeddedRefRelocator emb_reloc;
>       iterate_roots(&emb_reloc);
>     }
>     {
>       tty->print_cr("Relocating external roots ... ");
>       ResourceMark rm;
>       RefRelocator ext_reloc;
>       iterate_roots(&ext_reloc);
>     }
>
>
>> src/share/vm/classfile/symbolTable.cpp
>>
>> - Line 84, could you please add a comment explain the change?
>>
>
> I reverted the change. During dump time, the _shared_table is empty so this is a safe no-op.
>
>> 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?
>>
>
> Fixed.
>
>> 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.
>
> Fixed
>
> Thanks again!
>
> - Ioi
>>
>> Thanks,
>> Jiangli
>>
>>> On Jun 29, 2017, at 3:55 PM, Ioi Lam <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8072061 <https://bugs.openjdk.java.net/browse/JDK-8072061>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v10/ <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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Ioi Lam
Hi Jiangli,


Sounds good. I'll put the comments where you suggested. E.g.

class ConstantPool : public Metadata {
   friend class VMStructs;
   friend class JVMCIVMStructs;
   friend class BytecodeInterpreter;  // Directly extracts a klass in
the pool for fast instanceof/checkcast
   friend class Universe;             // For null constructor
  private:
+ // If you add a new field that points to any metaspace object, you
must add this field to
+ // ConstantPool::metaspace_pointers_do().
   Array<u1>*           _tags;        // the tag array describing the
constant pool's contents


Thanks
- Ioi

On 7/28/17 10:54 AM, Jiangli Zhou wrote:

> Hi Ioi,
>
> Thank you so much for making the additional changes. I’d suggest to
> add the ‘warning’ comment above the first metadata field in each class
> containing metaspace_pointers_do(). No need for new webrev for the
> comment changes.
>
> Thanks!
>
> Jiangli
>
>> On Jul 27, 2017, at 12:24 PM, Ioi Lam <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi Jiangli,
>>
>> Thanks for the very detailed review! Please see my responses in-line
>>
>> I have uploaded a delta from the previous webrev at
>>
>> http://cr.openjdk.java.net/~iklam/jdk10/8072061-auto-size-cds-region.v11.delta/
>>
>> On 7/26/17 2:59 PM, Jiangli Zhou wrote:
>>> 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.
>>
>> Can you suggest a place for adding such comments?
>> <Class>::metaspace_pointers_do() doesn't seem to be the right place
>> -- if you're reading this function, you should already know what
>> you're supposed to do.
>>
>> I think it's fairly easy to crash in the CDS tests if you missed a
>> field -- the JVM would dereference an invalid pointer. However, when
>> such crash happens, I don't know what's the best way to tell the
>> developer that a field might be missed in metaspace_pointers_do().
>>
>>> Also, it might be a good idea to run the IsRefInArchiveChecker for
>>> non-debug build. How slow is the check?
>>
>> It's not too bad, but it doesn't help with the case of missing
>> fields. We just check that for all fields that you have remembered to
>> scan, they point to good addresses. This check is for the
>> implementation of ArchiveCompactor, not for the completeness of the
>> metaspace_pointers_do() functions.
>>
>>>
>>> src/share/vm/memory/metaspaceShared.cpp
>>>
>>> - Line 77, od should stand for optional data.
>>
>> Fixed.
>>
>>> I also have a question. We had planned to make the od region
>>> optionally mapped. Would that still be doable with the change?
>>>
>>
>> Yes. This change only moves the location of the OD section and
>> doesn't affect it in any other ways.
>>
>>> - Line 148, can you please change it into multiple lines following
>>> our coding convention.
>>> 148 if (total == 0) {total = 1;}
>>
>> Fixed.
>>
>>> - 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.
>>>
>>
>> How about this comment?
>>
>> /*
>>  * On 64-bit VM, the heap and class space layout will be the same as if
>>  * you're running in -Xshare:on mode:
>>  *
>>  * +-- SharedBaseAddress (default = 0x800000000)
>>  *                         v
>>  * +-..---------+----+ ... +----+----+----+----+----+---------------+
>>  * |    Heap    | ST |     | MC | RW | RO | MD | OD | class space   |
>>  * +-..---------+----+ ... +----+----+----+----+----+---------------+
>>  * |<--MaxHeapSize->|     |<-- UnscaledClassSpaceMax = 4GB ------->|
>>  *
>>  */
>>
>>> - 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.
>>>
>>
>> I changed it to vm_exit_during_initialization, as there isn't much we
>> can when you completely run out of memory in the current process.
>>
>>> - Line 823, should the assert be uncommented?
>>>
>>
>> Fixed. There was a bug in calculating the RO sizes, but I fixed that
>> too. See ArchiveCompactor::allocate() in the webrev.
>>
>>> - Line 877, could you please move ArchiveCompactor class to it’s own
>>> file?
>>>
>>
>> I agree that metaspaceShared.cpp is getting too big. However, I've
>> made a lot of changes in this file already, and I want to keep track
>> of the changes in the repo's history. If I move ArchiveCompactor now,
>> I need to change other parts of metaspaceShared.cpp, and there will
>> be confusion about what change is for implementing this RFE, and what
>> changes are related to the cleanup.
>>
>> I think it's better to do the cleanup of metaspaceShared.cpp in a
>> separate RFE, so we just change one thing at a time.
>>
>>> - Line 891, how about rename MyTable to RelocationCache?
>>>
>>
>> Fixed. I changed it to RelocationTable since it's a type of
>> ResourceHashtable.
>>
>>> - Line 909 & 911, can allocation in RO/RW region fail? If so, we
>>> need to handle the failure case.
>>>
>>
>> It will never fail. The only failure is failure to commit memory,
>> which will cause VM to exit.
>>
>>> - 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.
>>>
>>
>> I changed the calculation of the string regions so now the output
>> looks like this for a small number of strings:
>>
>>
>> mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4%
>> used] at 0x0000000800000000
>> rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9%
>> used] at 0x0000000800006000
>> ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0%
>> used] at 0x000000080040a000
>> md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4%
>> used] at 0x0000000800adf000
>> od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9%
>> used] at 0x0000000800ae1000
>> s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0%
>> used] at 0x00000007bfc00000
>> s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0%
>> used] at 0x0000000000000000
>> total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9%
>> used]
>>
>>
>> and this for a large number of strings. (I added a new stress test
>> case, but unfortunately that needed to be a closed test.).
>>
>>
>> mc space:     21736 [  0.1% of total] out of     24576 bytes [ 88.4%
>> used] at 0x0000000800000000
>> rw space:   4208160 [ 23.7% of total] out of   4210688 bytes [ 99.9%
>> used] at 0x0000000800006000
>> ro space:   7163592 [ 40.3% of total] out of   7163904 bytes [100.0%
>> used] at 0x000000080040a000
>> md space:      6016 [  0.0% of total] out of      8192 bytes [ 73.4%
>> used] at 0x0000000800adf000
>> od space:   6357656 [ 35.7% of total] out of   6361088 bytes [ 99.9%
>> used] at 0x0000000800ae1000
>> s0 space:     16384 [  0.1% of total] out of     16384 bytes [100.0%
>> used] at 0x00000007bfc00000
>> s1 space:         0 [  0.0% of total] out of         0 bytes [  0.0%
>> used] at 0x0000000000000000
>> total   :  17773544 [100.0% of total] out of  17784832 bytes [ 99.9%
>> used]
>>
>> The calculation is done inside FileMapInfo::write_string_regions, so
>> I don't have to duplicated the logic else where. I keep the use of
>> DumpRegion for _s0_region and _s1_region so that the size statistics
>> printing code can be uniform.
>>
>> By the way, I found the logic in FileMapInfo::write_string_regions a
>> little hard to understand, so I added these comments:
>>
>> // Here's the mapping from (GrowableArray<MemRegion> *regions) ->
>> (metaspace string regions).
>> //   + We have 1 or more heap regions: r0, r1, r2 ..... rn
>> //   + We have 2 metaspace string regions: s0 and s1
>> //
>> // If there's a single heap region (r0), then s0 == r0, and s1 is empty.
>> // Otherwise:
>> //
>> // "X" represented space that's occupied by heap objects.
>> // "_" represented unused spaced in the heap region.
>> //
>> //
>> //    |r0        | r1  | r2 | ...... | rn |
>> //    |XXXXXX|__ |XXXXX|XXXX|XXXXXXXX|XXXX|
>> //    |<-s0->|   |<- s1 ----------------->|
>> //            ^^^
>> //             |
>> //             +-- unmapped space
>>
>>> - 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.
>>>
>>
>> "Relocate" means "update a pointer to point to the new location of a
>> copied object". Here the action is
>>
>>       relocate (the pointers in) SystemDictionary::_well_known_klasses[]
>>
>> I think "update" is too generic. The use of 'relocate' is consistent
>> with the rest of the operations
>>
>>     {
>>       tty->print_cr("Relocating embedded pointers ... ");
>>       ResourceMark rm;
>>       ShallowCopyEmbeddedRefRelocator emb_reloc;
>>       iterate_roots(&emb_reloc);
>>     }
>>     {
>>       tty->print_cr("Relocating external roots ... ");
>>       ResourceMark rm;
>>       RefRelocator ext_reloc;
>>       iterate_roots(&ext_reloc);
>>     }
>>
>>
>>> src/share/vm/classfile/symbolTable.cpp
>>>
>>> - Line 84, could you please add a comment explain the change?
>>>
>>
>> I reverted the change. During dump time, the _shared_table is empty
>> so this is a safe no-op.
>>
>>> 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?
>>>
>>
>> Fixed.
>>
>>> 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.
>>
>> Fixed
>>
>> Thanks again!
>>
>> - Ioi
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>> On Jun 29, 2017, at 3:55 PM, Ioi Lam <[hidden email]
>>>> <mailto:[hidden email]>> 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
>>>
>>
>

Loading...