RFR(M): 8148195: Some InstanceKlass and MethodCounters fields can be excluded when JVMTI is not supported

[Coleen Phillimore]

http://cr.openjdk.java.net/~cjplummer/8148195/webrev.02/webrev.hotspot/src/share/vm/oops/method.hpp.udiff.html

Can you add // INCLUDE_JVMTI to this #endif around line 1069.

My threshold for missing // INCLUDE comment at the end of #endif is 
about 4 lines.

I think this looks good though.  It should save a bit of memory and is 
not too invasive.

thanks,
Coleen

On 4/4/16 12:09 PM, Chris Plummer wrote:
> 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.spitsyn at 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
>>>
>>
>