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
