[9] RFR (S) 8148639: Some MethodCounter fields can be excluded when not including C2

[Chris Plummer]

Thanks Vladimir!

I need one more reviewer please.

thanks,

Chris

On 3/28/16 10:52 AM, Vladimir Kozlov wrote:
> Looks good.
>
> Thanks,
> Vladimir
>
> On 3/28/16 10:35 AM, Chris Plummer wrote:
>> On 3/25/16 1:49 PM, Vladimir Kozlov wrote:
>>> On 3/25/16 12:45 PM, Chris Plummer wrote:
>>>> Hi Vladimir,
>>>>
>>>> [Sorry about the delay. I was looking on hotspot-comp for comments. 
>>>> Forgot I also posted to hotspot-dev.]
>>>>
>>>> Yes, I can make that change and was already debating between two 
>>>> approaches. Do you also want me to pull
>>>> interpreter_invocation_counter_offset() and 
>>>> interpreter_invocation_counter_offset_in_bytes() into the main
>>>> #if/#else/#endif section? I think maybe they are best left as-is or 
>>>> they will be separated from other XXX_offset()
>>>> methods.
>>>
>>> Move them together and use their own #if/#else/#endif section.
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8148639/webrev.04/webrev.hotspot/
>>
>> thanks,
>>
>> Chris
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>
>>>> On 3/22/16 3:11 PM, Vladimir Kozlov wrote:
>>>>> About changes. I don't like changes in methodCounters.hpp (other 
>>>>> files are fine).
>>>>>
>>>>> May be combine methods under one #if instead of separate #if per 
>>>>> method.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 3/22/16 3:01 PM, Vladimir Kozlov wrote:
>>>>>> On 3/22/16 2:10 PM, Chris Plummer wrote:
>>>>>>> Hi Vladimir,
>>>>>>>
>>>>>>> I just noticed that there are already about 50 occurrences of:
>>>>>>>
>>>>>>> #if defined(COMPILER2) || INCLUDE_JVMCI
>>>>>>
>>>>>> I know about that. I thought I can use you to fix it ;)
>>>>>>
>>>>>>>
>>>>>>> Should I just leave my changes as-is to be consistent?
>>>>>>
>>>>>> Okay, if you don't want to do that I am fine with that.
>>>>>>
>>>>>> I filed RFE:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8152470
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 3/21/16 6:09 PM, Chris Plummer wrote:
>>>>>>>> Ok. That sounds like a good idea.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 3/21/16 5:02 PM, Vladimir Kozlov wrote:
>>>>>>>>> Chris can you also replace:
>>>>>>>>>
>>>>>>>>> #if defined(COMPILER2) || INCLUDE_JVMCI
>>>>>>>>>
>>>>>>>>> with
>>>>>>>>>
>>>>>>>>> #if COMPILER2_OR_JVMCI
>>>>>>>>>
>>>>>>>>> where
>>>>>>>>>
>>>>>>>>> // COMPILER2 or JVMCI
>>>>>>>>> #if defined(COMPILER2) || INCLUDE_JVMCI
>>>>>>>>> #define COMPILER2_OR_JVMCI 1
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 3/21/16 4:53 PM, Chris Plummer wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review the following for removing a couple of 
>>>>>>>>>> MethodCounter fields when not including C2 (or JVMCI) in the
>>>>>>>>>> build.
>>>>>>>>>> This helps reduce footprint for the minimal VM (and the 
>>>>>>>>>> client VM also).
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8148639/webrev.03/webrev.hotspot/ 
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8148639
>>>>>>>>>>
>>>>>>>>>> There were a couple of changes the previously were in 
>>>>>>>>>> JDK-8147978 that I had to add to this webrev since
>>>>>>>>>> JDK-8147978 was
>>>>>>>>>> backed out. They include making ProfileInterpreter related 
>>>>>>>>>> options unsupported when not using C2, and also the new
>>>>>>>>>> macros in macros.hpp.
>>>>>>>>>>
>>>>>>>>>> In order to make sure these counters really are not used when 
>>>>>>>>>> not using C2, I took a few safeguards. The first
>>>>>>>>>> was to
>>>>>>>>>> make no changes other than to assert that whenever these 
>>>>>>>>>> counters are fetched, they are equal to zero. I did
>>>>>>>>>> quite a bit
>>>>>>>>>> of testing with this and never hit the asserts.
>>>>>>>>>>
>>>>>>>>>> I would have liked to #ifdef out 
>>>>>>>>>> interpreter_invocation_count() and 
>>>>>>>>>> interpreter_throwout_count(), but there are
>>>>>>>>>> too many
>>>>>>>>>> places that call them, which meant too many #ifdef in IMHO. 
>>>>>>>>>> So instead of #ifdef'ing them out, I just make them
>>>>>>>>>> return 0
>>>>>>>>>> when not using C2. This is safe because of the assert testing 
>>>>>>>>>> I did above.
>>>>>>>>>>
>>>>>>>>>> I do completely #ifdef out the two increment methods. 
>>>>>>>>>> interpreter_throwout_increment() is only called by some
>>>>>>>>>> ProfileInterpreter code in bytecodeInterpreter.cpp, so I 
>>>>>>>>>> #ifdef'd that code also. In interpreterRuntime.cpp I
>>>>>>>>>> #ifdef'd
>>>>>>>>>> out a call to interpreter_throwout_increment(). Although this 
>>>>>>>>>> code may have been executed when not using C2, the
>>>>>>>>>> assert
>>>>>>>>>> testing I did above showed that if the increment happened, 
>>>>>>>>>> the counter was never used later.
>>>>>>>>>>
>>>>>>>>>> There are quite a few #ifdefs in methodCounters.hpp. I could 
>>>>>>>>>> collpase 5 into one big #if/#else/#endif section for
>>>>>>>>>> all
>>>>>>>>>> the inline method implementations. It looks cleaner, be then 
>>>>>>>>>> also puts distance between the two different
>>>>>>>>>> versions of
>>>>>>>>>> the same method.
>>>>>>>>>>
>>>>>>>>>> Testing was done with jprt "-testset hotspot". I also did a 
>>>>>>>>>> lot of testing with various tools, svc, and compiler
>>>>>>>>>> test
>>>>>>>>>> lists, and also runThese. This was done on linux-x86 with 
>>>>>>>>>> "-client -Xcomp" and just "-client", and also on
>>>>>>>>>> linux-x64
>>>>>>>>>> with "-server -XX:+TieredCompilation" (although I think that 
>>>>>>>>>> was probably the default anyway).
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>
>>>>>>>
>>>>
>>