RFR[S] 8209657 Refactor filemap.hpp to simplify integration with Serviceability Agent
Ioi Lam
ioi.lam at oracle.com
Tue Aug 21 22:05:35 UTC 2018
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 at 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.
>>>>>
>>>>>
>>>>
>>>
>>
More information about the serviceability-dev
mailing list