RFR[S] 8209657 Refactor filemap.hpp to simplify integration with Serviceability Agent
Calvin Cheung
calvin.cheung at oracle.com
Tue Aug 21 16:54:14 UTC 2018
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