RFR (XS) 8145593 - Clean up metaspaceShared.cpp

[Jiangli Zhou]

Hi Ioi,

Looks good in general.

- src/share/vm/memory/metaspaceShared.cpp

The MetaspaceShared::set_shared_rs() is only called from VirtualSpaceNode::VirtualSpaceNode() when DumpSharedSpaces is true. You can change ‘if (DumpSharedSpaces)’ condition to an assert(). Also, it might be better to rename set_shared_rs() to be initialize_shared_rs() since now it’s doing much more than just setting the ‘_shared_rs’.

For the following loop, 
 678   for (int pass=1; pass<=2; pass++) {
 679     if (pass == 1) {
 680       // The first pass doesn't actually write the data to disk. All it
 681       // does is to update the fields in the mapinfo->_header.
 682     } else {
 683       // After the first pass, the contents of mapinfo->_header are finalized,
 684       // so we can compute the header's CRC, and write the contents of the header
 685       // and the regions into disk.
 686       mapinfo->open_for_write();
 687       mapinfo->set_header_crc(mapinfo->compute_header_crc());
 688     }
 689     mapinfo->write_header();
 690     mapinfo->write_space(MetaspaceShared::ro, _loader_data->ro_metaspace(), true);
 691     mapinfo->write_space(MetaspaceShared::rw, _loader_data->rw_metaspace(), false);
 692     mapinfo->write_region(MetaspaceShared::md, _md_vs.low(),
 693                           pointer_delta(md_top, _md_vs.low(), sizeof(char)),
 694                           SharedMiscDataSize,
 695                           false, false);

it might simplify the code a little bit if it’s changed to:

 678   for (int pass=1; pass<=2; pass++) {
 679     if (pass == 2) {
 680       // The first pass doesn't actually write the data to disk. All it
 681       // does is to update the fields in the mapinfo->_header.
           //
 683       // After the first pass, the contents of mapinfo->_header are finalized,
 684       // so we can compute the header's CRC, and write the contents of the header
 685       // and the regions into disk.
 686       mapinfo->open_for_write();
 687       mapinfo->set_header_crc(mapinfo->compute_header_crc());
 688     }
 689     mapinfo->write_header();
 690     mapinfo->write_space(MetaspaceShared::ro, _loader_data->ro_metaspace(), true);
 691     mapinfo->write_space(MetaspaceShared::rw, _loader_data->rw_metaspace(), false);
 692     mapinfo->write_region(MetaspaceShared::md, _md_vs.low(),
 693                           pointer_delta(md_top, _md_vs.low(), sizeof(char)),
 694                           SharedMiscDataSize,
 695                           false, false);

Thanks,
Jiangli


> On Dec 16, 2015, at 11:37 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> Please review a small fix:
> 
> http://cr.openjdk.java.net/~iklam/jdk9/8145593-metaspace-shared-cleanup.v01/
> 
> Bug: Clean up metaspaceShared.cpp
> 
>    https://bugs.openjdk.java.net/browse/JDK-8145593
> 
> Summary of fix:
> 
>   [1] The "md" and "mc" spaces are managed inside VM_PopulateDumpSharedSpace,
>   which makes the implementation of MetaspaceShared::misc_data_space_alloc()
>   very awkward. The relevant code should be moved into the MetaspaceShared class.
> 
>   [2] There is a block of code that appears twice in VM_PopulateDumpSharedSpace::doit().
>   It should be cleaned up using a 2-iteration loop.
> 
> Tests:
> 
>    JPRT
>    CDS tests (locally)
> 
> Thanks
> - Ioi