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

[Zoltán Majó]

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