RFR (L) 8072061: Automatically determine optimal sizes for the CDS regions
Jiangli Zhou
jiangli.zhou at oracle.com
Fri Jul 28 17:54:15 UTC 2017
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 <ioi.lam at oracle.com> 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 <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> 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
>>
>
More information about the hotspot-runtime-dev
mailing list