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

[Calvin Cheung]

Hi Ioi,

This is a very nice cleanup and looks good.

In the SA native code, I'm wondering instead of:

#include "../../../../hotspot/share/include/cds.h"

is it possible to omit the relative path and just have the following?

#include "cds.h"

I think it probably involves some makefile changes.
It's fine to leave it as is or fix it in another bug.

thanks,
Calvin

On 8/21/18, 6:19 AM, Ioi Lam wrote:

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.