RFR (tedious) 8230586 Encapsulate fields in filemap.hpp

[Calvin Cheung]

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
>