[9] RFR(S): 8059606: Enable per-method usage of CompileThresholdScaling (per-method compilation thresholds)

[Zoltán Majó]

On 01/16/2015 07:34 PM, Vladimir Kozlov wrote:
> Looks good. You need second review for this - it is not small :)

thank you, Vladimir, for the review.

You're right, this change started small and got bigger, so it's 
definitely not small anymore. But I won't change the subject now so that 
we don't move discussion to a different thread.

Best regards,


Zoltan


>
> Thanks,
> Vladimir
>
> On 1/16/15 5:05 AM, Zoltán Majó wrote:
>> Hi Vladimir,
>>
>>
>> thank you for the feedback!
>>
>> On 01/13/2015 08:09 PM, Vladimir Kozlov wrote:
>>> Thank you, Zoltan, for performance testing!
>>>
>>> templateInterpreter_sparc.cpp - why you switched from G3 to G1 in 
>>> 325,344 lines?
>>
>> The reason is that the register Rcounter, which is used to store the 
>> address of MethodCounters, is an alias of G3.
>>
>> In the old code, we do not need MethodCounters after incrementing the 
>> invocation counter (lines 317--326), so that is
>> why it is OK that the instruction
>>
>> 331: __ load_contents(profile_limit, G3_scratch);
>>
>> overwrites the contents of G3.
>>
>> In the new version, however, we do need MethodCounters afterwards 
>> (e.g., lines 339--344), so that is why I thought we
>> should use G1 instead of G3.
>>
>>>
>>> templateTable_x86_64.cpp - indention:
>>>
>>>         __ movptr(rcx, Address(rcx, Method::method_counters_offset()));
>>> +         const Address mask(rcx, 
>>> in_bytes(MethodCounters::backedge_mask_offset()));
>>
>> Thanks for noticing, I've corrected the indentation.
>>
>>> Can you move new code in method.cpp into MethodCounters() 
>>> constructor? I don't see why it should be in method.cpp.
>>
>> I did that.
>>
>>>
>>> advancedThresholdPolicy.hpp - some renaming left.
>>
>> Updated.
>>
>> Here is the new webrev: 
>> http://cr.openjdk.java.net/~zmajo/8059606/webrev.02/
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>
>>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 1/13/15 4:31 AM, Zoltán Majó wrote:
>>>> Hi Vladimir,
>>>>
>>>>
>>>> thank you for the feedback! Please see comments below.
>>>>
>>>> On 01/05/2015 08:13 PM, Vladimir Kozlov wrote:
>>>>> On 1/5/15 10:38 AM, Zoltán Majó wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> please review the following patch.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8059606
>>>>>>
>>>>>> Problem: Controlling compilation thresholds on a per-method level 
>>>>>> can
>>>>>> be useful for debugging and understanding
>>>>>> failures, but currently there is no way to control on a per-method
>>>>>> level when methods are compiled.
>>>>>>
>>>>>>
>>>>>> Solution:
>>>>>>
>>>>>> This patch adds support for scaling compilation thresholds on a
>>>>>> per-method level using the CompileThresholdScaling flag.
>>>>>> For example, the option
>>>>>>
>>>>>> -XX:CompileCommand=option,SomeClass.someMethod,double,CompileThresholdScaling,0.5 
>>>>>>
>>>>>>
>>>>>>
>>>>>> reduces compilation thresholds for method SomeClass.sometMethod() by
>>>>>> 50% (but leaves global thresholds unaffected) and
>>>>>> results in earlier compilation of the method.
>>>>>>
>>>>>> Similar to the global CompileThresholdScaling flag (added in
>>>>>> JDK-805604), the per-method CompileThresholdScaling flag
>>>>>> works with both tiered and non-tiered modes of operation.
>>>>>>
>>>>>> Per-method compilation thresholds are available only in non-product
>>>>>> builds to avoid the overhead of accessing fields
>>>>>> added by the patch MethodData and MethodCounters.
>>>>>
>>>>> Too many ifdefs :)
>>>>
>>>> I made per-method compilation thresholds available in product 
>>>> builds as
>>>> well. That helps reducing the number of ifdefs :-).
>>>>
>>>>> The interpreter speed is not important. And the feature could be
>>>>> interesting in product VM too.
>>>>> The only drawback is 2 additional fields in MDO which is fine.
>>>>> Can you make it product and run through our performance 
>>>>> infrastructure.
>>>>
>>>> Performance data show that per-method compilation thresholds do not
>>>> result in a statistically significant change of performance. One
>>>> benchmark, Footprint3-Client, degrades ~0.5% on the X86 Client VM, 
>>>> but I
>>>> think that is negligible.
>>>>
>>>>> Also, as John Rose will say, we should have as much as possible a
>>>>> similar code in product as in tested debug code. Otherwise we are not
>>>>> testing product bits and will get into troubles.
>>>>>
>>>>>>
>>>>>> The proposed patch supports x86_64, x86_32, and sparc. Do you think
>>>>>> it is necessary to support other architectures as well?
>>>>>
>>>>> Yes. It should be supported on all platforms.
>>>>
>>>> The current patch supports all architectures except PPC64.
>>>>
>>>>>
>>>>>> The patch updates the name of the flags Tier2BackEdgeThreshold,
>>>>>> Tier3BackEdgeThreshold, Tier4BackEdgeThreshold
>>>>>> (lowercase e in "Back*e*dge) so that the naming is consistent with
>>>>>> other backedge-related flags
>>>>>> (Tier0BackedgeNotifyFreqLog, Tier2BackedgeNotifyFreqLog, and
>>>>>> Tier3BackedgeNotifyFreqLog).
>>>>>
>>>>> It added noise to main changes and may cause some testing (jfr?)
>>>>> failures. Can we do it separately (other RFE?).
>>>>
>>>> I created issue 8068506 for that.
>>>>
>>>> Here is the new webrev:
>>>> http://cr.openjdk.java.net/~zmajo/8059606/webrev.01/
>>>>
>>>> Testing: manual testing, JPRT
>>>>
>>>>>> This patch is the third (and final) part of JDK-8050853:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8050853 .
>>>>>>
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~zmajo/8059606/webrev.00/
>>>>>
>>>>> In general looks good.
>>>>
>>>> Thank you and best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> Testing: manual testing on all supported architectures, JPRT.
>>>>>>
>>>>>> Thank you and best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>
>>