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

[Chris Plummer]

Thanks Coleen!

Chris

On 3/28/16 12:00 PM, Coleen Phillimore wrote:
>
> This looks good to me.
> Coleen
>
> On 3/28/16 2:21 PM, Chris Plummer wrote:
>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>
>>
>