RFR (tedious) 8230586 Encapsulate fields in filemap.hpp

[Ioi Lam]

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