[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(M): 8148195: Some InstanceKlass and MethodCounters fields can be excluded when JVMTI is not
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2016-03-31 16:31:01
Message-ID: 56FD50C5.4020303 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Serguei,<br>
<br>
Thanks for the review. I'll make all the changes you suggested.<br>
<br>
cheers,<br>
<br>
Chris<br>
<br>
On 3/31/16 12:38 AM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote cite="mid:56FCD404.8030601@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Chris,<br>
<br>
<br>
It looks pretty good, thanks!<br>
<br>
Just some minor comments below.<br>
<br>
src/share/vm/ci/ciMethod.cpp<br>
<br>
The only file with old Copyright year.<br>
<br>
src/share/vm/oops/instanceKlass.hpp<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<pre> 907 // Breakpoint support (see methods on Method* for details)
<span class="new"> 908 #if INCLUDE_JVMTI</span>
909 BreakpointInfo* breakpoints() const { return _breakpoints; };
910 void set_breakpoints(BreakpointInfo* bps) { _breakpoints = bps; };
<span class="new"> 911 #endif</span></pre>
Better to move the comment at the L907 inside the ifdef block.<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<pre>1280 // RedefineClasses support
<span class="new">1281 #if INCLUDE_JVMTI</span>
1282 void link_previous_versions(InstanceKlass* pv) { _previous_versions = pv; }
1283 void mark_newly_obsolete_methods(Array<Method*>* old_methods, int \
emcp_method_count); <span class="new">1284 #endif</span></pre>
Better to move the comment at the L1280 inside the ifdef block.<br>
<br>
<br>
src/share/vm/oops/method.hpp<br>
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<pre>1041 /// Fast Breakpoints.
1042
1043 // If this structure gets more complicated (because bpts get numerous),
1044 // move it into its own header.
1045
1046 // There is presently no provision for concurrent access
1047 // to breakpoint lists, which is only OK for JVMTI because
1048 // breakpoints are written only at safepoints, and are read
1049 // concurrently only outside of safepoints.
1050
<span class="new">1051 #if INCLUDE_JVMTI</span></pre>
Better to move the comments at the L1041-1949 inside the ifdef
block.<br>
<br>
Consider it reviewed, I do not need to see another webrev.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 3/30/16 23:20, Chris Plummer wrote:<br>
</div>
<blockquote cite="mid:56FCC1B9.3060007@oracle.com" type="cite">Please
review the following for removing some fields that are not
needed when not supporting JVMTI. <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8148195">https://bugs.openjdk.java.net/browse/JDK-8148195</a>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8148195/webrev.02/webrev.hotspot/">http://cr.openjdk.java.net/~cjplummer/8148195/webrev.02/webrev.hotspot/</a>
<br>
<br>
I had passed a preliminary review around a month or so ago. The
webrev is here: <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8148195/webrev.01/webrev.hotspot/">http://cr.openjdk.java.net/~cjplummer/8148195/webrev.01/webrev.hotspot/</a>
<br>
<br>
I made a number of changes since then. I tried to reduce the
number of #ifdefs, but at the same time include less unnecessary
code in the INCLUDE_JVMTI=0 build. For example, BreakpointInfo
is now completely gone when not including JVMTI. I didn't really
succeed at the former since #ifdefs seem to have just moved
around, but there is a lot more code conditionally compiled out
now, and I think it's cleaner this way. <br>
<br>
Also since the previous webrev, I added some fixes for SA,
although these aren't possible to test right now. Currently the
minimal VM is the only one that supports excluding JVMTI, but it
also excludes SA, so that makes it hard to test conditionally
removing some JVMTI support from SA. I tried doing a client VM
build without JVMTI, but that's currently broken (can't build
with INCLUDE_JVMTI=0 and INCLUDE_SERVICES=1). It's a known issue
that's already being worked on. <br>
<br>
Testing I've done: <br>
<br>
- jprt -testset hotspot <br>
- jck vm tests with minimal vm on linux-x86 <br>
- hotspot/test/:compact2_minimal with minimal vm on linux-x86 <br>
- all the serviceability tests I could find supported by RBT.
Ran with client vm <br>
on linux-x86 and server vm on linux-x64. <br>
<br>
I'm going to try to do more minimal VM testing. I need to figure
out more test suites I can run with minimal and not get a ton of
errors due to the tests using excluded functionality. <br>
<br>
thanks, <br>
<br>
Chris <br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic