RFR (L) 8072061: Automatically determine optimal sizes for the CDS regions
Ioi Lam
ioi.lam at oracle.com
Thu Jul 27 19:24:51 UTC 2017
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