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

[Chris Plummer]

Hi Coleen,

Yep, I can do that. Although my threshold is a bit more than 4 lines, I 
normally would have put the comment on this one but I missed it.

thanks,

Chris

On 4/4/16 10:51 AM, Coleen Phillimore wrote:
> 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
>>>>
>>>
>>
>