RFR (L) 8072061: Automatically determine optimal sizes for the CDS regions
Ioi Lam
ioi.lam at oracle.com
Fri Jul 28 18:12:49 UTC 2017
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 <ioi.lam at oracle.com
>> <mailto: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/
>>
>> 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
>>>> 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