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

[Chris Plummer]

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
>>
>