RFR (XS) 8145593 - Clean up metaspaceShared.cpp

[Ioi Lam]

Hi Jiangli,

Thanks for the review

On 12/16/15 4:48 PM, Jiangli Zhou wrote:
> 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’.
OK, I will change as you suggested.
> 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);

I prefer the first style, as it's more explicit about what's happening 
in pass #1 vs pass #2. Otherwise you have to read the entire comment and 
them try to figure out which part pertains to pass #1 vs #2.

Thanks
Ioi

> Thanks,
> Jiangli
>
>
>> On Dec 16, 2015, at 11:37 AM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>> Please review a small fix:
>>
>> http://cr.openjdk.java.net/~iklam/jdk9/8145593-metaspace-shared-cleanup.v01/ 
>> <http://cr.openjdk.java.net/%7Eiklam/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
>