Ping! Still looking for one more reviewer.
thanks,
Chris
On 3/31/16 9:31 AM, Chris Plummer wrote:
Hi Serguei,
Thanks for the review. I'll make all the changes you suggested.
cheers,
Chris
On 3/31/16 12:38 AM, serguei.spit...@oracle.com wrote:
Hi Chris,
It looks pretty good, thanks!
Just some minor comments below.
src/share/vm/ci/ciMethod.cpp
The only file with old Copyright year.
src/share/vm/oops/instanceKlass.hpp
907 // Breakpoint support (see methods on Method* for details)
908 #if INCLUDE_JVMTI
909 BreakpointInfo* breakpoints() const { return _breakpoints; };
910 void set_breakpoints(BreakpointInfo* bps) { _breakpoints = bps; };
911 #endif
Better to move the comment at the L907 inside the ifdef block.
1280 // RedefineClasses support
1281 #if INCLUDE_JVMTI
1282 void link_previous_versions(InstanceKlass* pv) { _previous_versions =
pv; }
1283 void mark_newly_obsolete_methods(Array<Method*>* old_methods, int
emcp_method_count);
1284 #endif
Better to move the comment at the L1280 inside the ifdef block.
src/share/vm/oops/method.hpp
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
1051 #if INCLUDE_JVMTI
Better to move the comments at the L1041-1949 inside the ifdef block.
Consider it reviewed, I do not need to see another webrev.
Thanks,
Serguei
On 3/30/16 23:20, Chris Plummer wrote:
Please review the following for removing some fields that are not
needed when not supporting JVMTI.
https://bugs.openjdk.java.net/browse/JDK-8148195
http://cr.openjdk.java.net/~cjplummer/8148195/webrev.02/webrev.hotspot/
I had passed a preliminary review around a month or so ago. The
webrev is here:
http://cr.openjdk.java.net/~cjplummer/8148195/webrev.01/webrev.hotspot/
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.
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.
Testing I've done:
- jprt -testset hotspot
- jck vm tests with minimal vm on linux-x86
- hotspot/test/:compact2_minimal with minimal vm on linux-x86
- all the serviceability tests I could find supported by RBT. Ran
with client vm
on linux-x86 and server vm on linux-x64.
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.
thanks,
Chris
