<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Ioi,<br>
<br>
Thank you for the update!<br>
It looks good to me.<br>
<br>
The following declarations can be aligned (no need in new webrev):<br>
<pre><span class="new"> 169 bool _file_open;</span>
<span class="new"> 170 int _fd;</span>
<span class="new"> 171 size_t _file_offset;</span></pre>
Thank you for the explanation on the typedef <span class="new">FileMapHeader
move suggestion.<br>
I suspected it to be the case.<br>
<br>
Thanks,<br>
Serguei<br>
</span><br>
On 8/21/18 06:19, Ioi Lam wrote:<br>
</div>
<blockquote type="cite"
cite="mid:67b008d2-5efb-97c1-4829-ffc00f458568@oracle.com">
<p>Hi Serguei,</p>
<p>Thanks for the review. Updated webrev at</p>
<p><a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v03/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v03/</a><br>
</p>
<br>
<div class="moz-cite-prefix">On 8/21/18 1:28 AM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:d02e1fe8-924f-afc3-c878-591176aae96e@oracle.com">
<div class="moz-cite-prefix">Hi Ioi,<br>
<br>
A couple of quick minor comments...<br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/hotspot/share/include/cds.h.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/hotspot/share/include/cds.h.html</a><br>
<br>
<pre> 31 // We should use only standard C types. Do bot use custom types such as bool, intx,</pre>
 A typo: bot => not<br>
<br>
</div>
</blockquote>
<br>
Fixed<br>
<br>
<blockquote type="cite"
cite="mid:d02e1fe8-924f-afc3-c878-591176aae96e@oracle.com">
<div class="moz-cite-prefix">
<pre> 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 };</pre>
 It is better to remove dots at the end of 55 and 56 to
follow the local file style.<br>
<br>
<br>
</div>
</blockquote>
Fixed<br>
<br>
<blockquote type="cite"
cite="mid:d02e1fe8-924f-afc3-c878-591176aae96e@oracle.com">
<div class="moz-cite-prefix"> Â This typedef can be moved from
saproc.cpp, */ps_core.c to cds.h:<span class="new"><br>
  +typedef struct CDSFileMapHeaderBase FileMapHeader;</span><br>
<br>
<br>
</div>
</blockquote>
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.<br>
<br>
To avoid confusion, I removed the typedef and changed the SA code
to use CDSFileMapHeaderBase throughout.<br>
<br>
<blockquote type="cite"
cite="mid:d02e1fe8-924f-afc3-c878-591176aae96e@oracle.com">
<div class="moz-cite-prefix"> <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c.frames.html</a><br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c.frames.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c.frames.html</a><br>
<pre><span class="changed"> 337 print_debug("%s has bad shared archive file magic number 0x%x, expecing 0x%x\n",
Original typo can be fixed: expecing => expecting
</span></pre>
<br>
</div>
</blockquote>
<br>
Fixed.<br>
<br>
Thanks<br>
- Ioi<br>
<br>
<blockquote type="cite"
cite="mid:d02e1fe8-924f-afc3-c878-591176aae96e@oracle.com">
<div class="moz-cite-prefix"> Thanks,<br>
Serguei<br>
<br>
<br>
On 8/20/18 13:23, Ioi Lam wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7882a2a6-8692-c2a9-1546-569b4aad5fad@oracle.com">Hi,
<br>
<br>
I've updated the webrev to merge with Calvin's change in the
latest repo. <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v02/</a>
<br>
<br>
Thanks <br>
<br>
- Ioi <br>
<br>
On 8/17/2018 2:22 PM, Ioi Lam wrote: <br>
<blockquote type="cite">[Resending to include bug number in
e-mail subject line] <br>
<br>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8209657"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8209657</a>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eiklam/jdk12/8209657-shared-FileMapHeader-decl.v01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~iklam/jdk12/8209657-shared-FileMapHeader-decl.v01/</a>
<br>
<br>
Summary: <br>
<br>
The CDS FileMapHeader type was big, and was duplicated 4
times in our sources. <br>
I moved the parts that's common to HotSpot and
Serviceability Agent into a new <br>
common header file, cds.h. <br>
<br>
I also did various clean up in filemap.cpp/hpp: <br>
<br>
- avoid using unwieldy nested types such as
FileMapInfo::FileMapHeader::space_info <br>
- added convenience function space_at(), so you have <br>
<br>
 struct FileMapInfo::FileMapHeader::space_info* si = <br>
       &_header->_space[i]; <br>
=> <br>
 CDSFileMapRegion* si = space_at(i); <br>
<br>
<br>
Testing: <br>
<br>
hs tiers 1,2,3 on all supported platforms. <br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>