RFR(S) 8007270: Make IsMethodCompilable test work with tiered

[Nils Eliasson]

You are right.

New rev:
http://cr.openjdk.java.net/~neliasso/8007270/webrev.07/

Thanks,
//N



On 2014-02-26 15:04, Igor Ignatyev wrote:
> Nils,
> I think it would be better to have
>> if (!Platform.isServer())
> instead of > +        // Only c2 compilations can be disabled through
> PerMethodRecompilationCutoff
>> +        if (com.oracle.java.testlibrary.Platform.isClient()) {
>> +            return;
>> +        }
> since we have 'Minimal VM' which also has only C1.
>
> Igor
>
> On 02/26/2014 03:14 PM, Nils Eliasson wrote:
>> Sorry, now nice and tidy. One curly brace still looks to have the wrong
>> indentation in the udiff. It has the right indentation in the changeset
>> and in the other diffs (sdiff line 102).
>>
>> http://cr.openjdk.java.net/~neliasso/8007270/webrev.06/
>>
>> Thanks,
>> //Nils
>>
>> On 2014-02-17 19:46, Vladimir Kozlov wrote:
>>> On 2/17/14 1:19 AM, Nils Eliasson wrote:
>>>> New:
>>>>
>>>> http://cr.openjdk.java.net/~neliasso/8007270/webrev.05/
>>>>
>>>> Answers inline.
>>>>
>>>> On 2014-02-12 22:41, Vladimir Kozlov wrote:
>>>>> Indention is slightly off, otherwise new code is good. Can you shift
>>>>> code a little?:
>>>>>
>>>>> +        for (long attempts = 0, successes = 0;
>>>>> +             (successes < PER_METHOD_RECOMPILATION_CUTOFF) &&
>>>>> +             (attempts  < PER_METHOD_RECOMPILATION_CUTOFF*2) &&
>>>>> +             isCompilable(COMP_LEVEL_FULL_OPTIMIZATION);
>>>>> +             attempts++) {
>>>>> +          if (compileAndDeoptimize() ==
>>>>> COMP_LEVEL_FULL_OPTIMIZATION) {
>>>>> +              successes++;
>>>>>            }
>>>>>         }
>>>
>>> The indent is not fixed.
>>>
>>>>>
>>>>> Do we need to assert (throw exception) after the loop when
>>>>> (successes != PER_METHOD_RECOMPILATION_CUTOFF)?
>>>>
>>>> No, that will be caught be the other tests. First that it is still
>>>> compilable, and then after one more compilation and
>>>> deoptimization, that it is no longer compilable.
>>>
>>> Okay.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>>> You left debugging output code in compileAndDeoptimize(). Do you
>>>>> want to keep it?
>>>>
>>>> It was supposed to be removed.
>>>>
>>>> Thanks,
>>>> Nils
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 2/12/14 1:46 AM, Nils Eliasson wrote:
>>>>>> I rewrote the code. It should be much cleared now.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~neliasso/8007270/webrev.04/
>>>>>>
>>>>>> Thanks,
>>>>>> //N
>>>>>>
>>>>>>
>>>>>> On 2014-02-11 23:10, Vladimir Kozlov wrote:
>>>>>>> Sorry, I missed that. That code is very messy. I think you should
>>>>>>> decrement 'i' instead and change comment:
>>>>>>>
>>>>>>> +        long cutoff = PER_METHOD_RECOMPILATION_CUTOFF;
>>>>>>> +        // deoptimize 'PerMethodRecompilationCutoff' times
>>>>>>> +        for (long i = 0L; (i < cutoff) &&
>>>>>>> isCompilable(COMP_LEVEL_FULL_OPTIMIZATION); ++i) {
>>>>>>> +          int level = compileAndDeoptimize();
>>>>>>> +          if (level < COMP_LEVEL_FULL_OPTIMIZATION) {
>>>>>>> +            // Add one iteration if compiled with a lower tier
>>>>>>> +            --i;
>>>>>>>            }
>>>>>>>          }
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 2/11/14 1:34 PM, Nils Eliasson wrote:
>>>>>>>> cutoff may increase if the method was compiled for a lower level
>>>>>>>> and we
>>>>>>>> want another attempt for full opt, the second check is to limit
>>>>>>>> the
>>>>>>>> amount of attempts.
>>>>>>>>
>>>>>>>> //N
>>>>>>>>
>>>>>>>> On 2014-02-11 20:06, Vladimir Kozlov wrote:
>>>>>>>>> (i <c) should be always true since (i < cutoff):
>>>>>>>>>
>>>>>>>>> +        for (long i = 0L; (i < cutoff) &&
>>>>>>>>> +              (i < PER_METHOD_RECOMPILATION_CUTOFF*2) &&
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 2/11/14 5:15 AM, Nils Eliasson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Thanks for your contribution Igor.
>>>>>>>>>>
>>>>>>>>>> After some additional testing I noticed some inconsistencies and
>>>>>>>>>> filed https://bugs.openjdk.java.net/browse/JDK-8034188
>>>>>>>>>>
>>>>>>>>>> This test now runs faster, and targets c2-compiles only, in
>>>>>>>>>> server
>>>>>>>>>> and with tiered.
>>>>>>>>>>
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~neliasso/8007270/webrev.02/
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> //Nils
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2014-01-29 11:11, Igor Ignatyev wrote:
>>>>>>>>>>> Hi Nils,
>>>>>>>>>>>
>>>>>>>>>>> you can skip running on client by yourself, see attached diff.
>>>>>>>>>>> Igor
>>>>>>>>>>>
>>>>>>>>>>> On 01/28/2014 07:02 PM, Nils Eliasson wrote:
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> I need a review for this change.
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~neliasso/8007270/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> This test was disabled since it didn't work very well with
>>>>>>>>>>>> tiered
>>>>>>>>>>>> (or
>>>>>>>>>>>> client). It tests the PerMethodRecompilationCutoff that was
>>>>>>>>>>>> introduced
>>>>>>>>>>>> to disable c2-compilations of a method when it has been
>>>>>>>>>>>> deoptimized
>>>>>>>>>>>> too
>>>>>>>>>>>> many times.  The bug report suggested we should disable c1
>>>>>>>>>>>> compilations
>>>>>>>>>>>> as well but I don't think that was the intent of the cutoff
>>>>>>>>>>>> feature.
>>>>>>>>>>>>
>>>>>>>>>>>> I have changed the following in the test
>>>>>>>>>>>> * skip test when running client only (not supported by jtreg
>>>>>>>>>>>> at the
>>>>>>>>>>>> moment)
>>>>>>>>>>>> * check what compilation level was used when compiling so
>>>>>>>>>>>> that it
>>>>>>>>>>>> can
>>>>>>>>>>>> keep track of the number of c2 compiles (and deopts)
>>>>>>>>>>>> correctly in
>>>>>>>>>>>> tiered
>>>>>>>>>>>> mode
>>>>>>>>>>>> * compile and deopt up to the cutoff limit only once
>>>>>>>>>>>> * added PerMethodRecompilationCutoff=4 flag to commandline to
>>>>>>>>>>>> reduce
>>>>>>>>>>>> wasted time in test (default 400)
>>>>>>>>>>>>
>>>>>>>>>>>> Now the test works and the running time has been reduced to
>>>>>>>>>>>> seconds
>>>>>>>>>>>> instead of minutes.
>>>>>>>>>>>>
>>>>>>>>>>>> Kind regards,
>>>>>>>>>>>> Nils Eliasson
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>