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

[Vladimir Kozlov]

Looks good. You need second review for this - it is not small :)

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