[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR[S] 8209657 Refactor filemap.hpp to simplify integration with Serviceability Agent
From:       Ioi Lam <ioi.lam () oracle ! com>
Date:       2018-08-21 22:05:35
Message-ID: e238252f-ea36-2ed5-6763-bba5f09c8b82 () oracle ! com
[Download RAW message or body]

Hi Calvin,

Thanks for the review.

I found that hotspot/share/include is already specified in the include 
path for libsaproc.so, so I changed all the 3 includes to

              include "cds.h"

I am running mach5 tier1 again to make sure it builds for all supported 
platforms.

Thanks

- Ioi



On 8/21/18 9:54 AM, Calvin Cheung wrote:
> 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.spitsyn@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.
> > > > > 
> > > > > 
> > > > 
> > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]