[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(M): 8148195: Some InstanceKlass and MethodCounters fields can be excluded when JVMTI is not 
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-03-31 7:38:44
Message-ID: 56FCD404.8030601 () oracle ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]