Re: 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.spit...@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.