RFR[S] 8209657 Refactor filemap.hpp to simplify integration with Serviceability Agent

[Ioi Lam]

Hi Serguei,

Thanks for the review. Updated webrev at

http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v03/


On 8/21/18 1:28 AM, serguei.spitsyn at oracle.com wrote:
> Hi Ioi,
>
> A couple of quick minor comments...
>
>
> http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/hotspot/share/include/cds.h.html
>
>    31 // We should use only standard C types. Do bot use custom types such as bool, intx,
>   A typo: bot => not
>

Fixed

>    54 struct CDSFileMapHeaderBase {
>    55   unsigned int _magic;           // identify file type.
>    56   int          _crc;             // header crc checksum.
>    57   int          _version;         // must be CURRENT_CDS_ARCHIVE_VERSION
>    58   struct CDSFileMapRegion _space[NUM_CDS_REGIONS];
>    59 };
>   It is better to remove dots at the end of 55 and 56 to follow the 
> local file style.
>
>
Fixed

>   This typedef can be moved from saproc.cpp, */ps_core.c to cds.h:
>    +typedef struct CDSFileMapHeaderBase FileMapHeader;
>
>
filemap.hpp declares a FileMapHeader type with many more fields that are 
used only by HotSpot and not by SA. That's why the struct is named 
CDSFileMapHeaderBase in cds.h.

To avoid confusion, I removed the typedef and changed the SA code to use 
CDSFileMapHeaderBase throughout.

>
> http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c.frames.html
> http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c.frames.html
> 337 print_debug("%s has bad shared archive file magic number 0x%x, 
> expecing 0x%x\n", Original typo can be fixed: expecing => expecting
>

Fixed.

Thanks
- Ioi

> Thanks,
> Serguei
>
>
> On 8/20/18 13:23, Ioi Lam wrote:
>> Hi,
>>
>> I've updated the webrev to merge with Calvin's change in the latest 
>> repo.
>>
>> http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/ 
>>
>>
>> Thanks
>>
>> - Ioi
>>
>> On 8/17/2018 2:22 PM, Ioi Lam wrote:
>>> [Resending to include bug number in e-mail subject line]
>>>
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8209657
>>> http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v01/ 
>>>
>>>
>>> Summary:
>>>
>>> The CDS FileMapHeader type was big, and was duplicated 4 times in 
>>> our sources.
>>> I moved the parts that's common to HotSpot and Serviceability Agent 
>>> into a new
>>> common header file, cds.h.
>>>
>>> I also did various clean up in filemap.cpp/hpp:
>>>
>>> - avoid using unwieldy nested types such as 
>>> FileMapInfo::FileMapHeader::space_info
>>> - added convenience function space_at(), so you have
>>>
>>>   struct FileMapInfo::FileMapHeader::space_info* si =
>>>         &_header->_space[i];
>>> =>
>>>   CDSFileMapRegion* si = space_at(i);
>>>
>>>
>>> Testing:
>>>
>>> hs tiers 1,2,3 on all supported platforms.
>>>
>>>
>>
>