RFR (tedious) 8230586 Encapsulate fields in filemap.hpp

[Ioi Lam]

Hi Calvin,

Thanks for the review. I've fixed both issues you mentioned. I'll do 
more testing before pushing the changes.

- Ioi

On 9/9/19 8:31 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> The updated webrev looks good.
>
> Just 2 nits in filemap.cpp
>
> 1561     address dumptime_heap_end = (address)header()->heap_end();
>
> Since heap_end() returns an address, the above typecast shouldn't be 
> needed.
>
> Please remove the extra blank line at line 180.
>
> I don't need to see another webrev for the above.
>
> thanks,
>
> Calvin
>
> On 9/9/19 6:56 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> thanks for the review. Please see my comments in-line:
>>
>> On 9/6/19 5:00 PM, Calvin Cheung wrote:
>>> Hi Ioi,
>>>
>>> Looks good overall.
>>>
>>> Just a few comments:
>>>
>>> filemap.hpp
>>> ----------
>>> 326   FileMapHeader *header() const       { return _header; }
>>>
>>> This may not be necessary because all references to header() are 
>>> from the class' member functions.
>>> e.g.
>>> 342   int    compute_header_crc()  const { return 
>>> header()->compute_crc(); }
>>>
>>> could be
>>>       int    compute_header_crc()  const { return 
>>> _header->compute_crc(); }
>>>
>>
>> In HotSpot, it's quite common to use the accessor function to access 
>> a class's own fields. E.g., see the calls to 
>> Klass::class_loader_data() in klass.cpp. This makes it easier to 
>> change the implementation of Klass::class_loader_data() when 
>> necessary, without changing every reference to _class_loader_data.
>>
>> In filemap.cpp, I try to remove all use of _xxxx fields, unless it's 
>> near where the field is written into. E.g.,
>>
>>   _header = (FileMapHeader*)os::malloc(header_size, mtInternal);
>>   memset((void*)_header, 0, header_size);
>>   _header->set_header_size(header_size);
>>
>>> 245 MemRegion heap_reserved()                const { return 
>>> _heap_reserved; }
>>>
>>> I couldn't find the _heap_reserved member.
>>>
>>
>> _heap_reserved was removed in JDK-8224815, which wasn't in my repo 
>> when I made the webrev. I have update the repo to use this instead:
>>
>>   address heap_end()                       const { return _heap_end; }
>>
>>
>>
>>> filemap.cpp
>>> ----------
>>> Extra blank line at line 180.
>>>
>>> You have 2 ways of accessing the _header:
>>> e.g.
>>> 196   _header->set_header_size(header_size);
>>>
>>> 214   header()->populate(this, alignment);
>>>
>>> As in the above comment on filemap.hpp, maybe just using _header is 
>>> fine.
>>>
>>
>> See my comments about accessor functions above.
>>
>>> cds.h
>>> ----
>>> 55   int        _is_heap_region;// used in debug build only.
>>>
>>> Needs a blank space after the ';'.
>>>
>> Fixed.
>>
>> Updated webrev is here:
>> http://cr.openjdk.java.net/~iklam/jdk14/8230586-encapsulate-filemap-hpp.v02/ 
>>
>>
>> Thanks
>> - Ioi
>>
>>> thanks,
>>>
>>> Calvin
>>>
>>> On 9/4/19 9:20 AM, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8230586
>>>> http://cr.openjdk.java.net/~iklam/jdk14/8230586-encapsulate-filemap-hpp.v01/ 
>>>>
>>>>
>>>> Many of the member fields in filemap.hpp are declared with public 
>>>> accessibility.
>>>> This is contrary to HotSpot coding style, and also makes the code
>>>> unnecessarily intertwined.
>>>>
>>>> I made the fields private and added accessor functions where 
>>>> necessary.
>>>>
>>>> Also, FileMapInfo::_header should not be accessed directly from 
>>>> other code.
>>>> I added accessor functions similar to FileMapInfo::core_spaces_size()
>>>> to access information stored in the header.
>>>>
>>>> Testing: hs-tier1, hs-tier2
>>>>
>>>> Thanks
>>>> - Ioi
>>