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

[Ioi Lam]

On 8/21/18 1:21 PM, serguei.spit...@oracle.com wrote:

Hi Ioi,

Thank you for the update!
It looks good to me.

The following declarations can be aligned (no need in new webrev):
169 bool _file_open;
170 int _fd;
171 size_t _file_offset;

Hi Serguei,

Thanks for the review. I've fixed the indent as you suggested.

Thanks
- Ioi

Thank you for the explanation on the typedef FileMapHeader move suggestion.

I suspected it to be the case.

Thanks,
Serguei

On 8/21/18 06:19, 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.