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

[serguei.spit...@oracle.com]

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; 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.