RFR(S): 8153013: BlockingCompilation test times out

[Christian Thalinger]

> On Apr 15, 2016, at 3:06 AM, Nils Eliasson <nils.eliasson at oracle.com> wrote:
> 
> Hi,
> 
> On 2016-04-14 20:45, Christian Thalinger wrote:
>> 
>>> On Apr 14, 2016, at 2:43 AM, Nils Eliasson < <mailto:nils.eliasson at oracle.com>nils.eliasson at oracle.com <mailto:nils.eliasson at oracle.com>> wrote:
>>> 
>>> I moved the reasons to CompileTask.hpp and put it together with the names list. Also changed the type from int to CompileReason as Igor suggested.
>>> 
>>> It gets verbose in the method declarations in compileBroker
>> 
>> Don’t worry about this.
>> 
>>> and sometimes I think CompileReason should be declared in CompileBroker because it is mostly used there. On the other hand, CompileTask is the keeper of the CompileReason so it makes sense too.
>> 
>> Yes, that’s the right place.
>> 
>>> 
>>> New webrev:
>>> http://cr.openjdk.java.net/~neliasso/8153013/webrev.03/ <http://cr.openjdk.java.net/%7Eneliasso/8153013/webrev.03/>
>> 
>> +   bool         can_become_stale() const          {
>> +     return !_is_blocking && (_compile_reason < Reason_Whitebox);
>> +   }
>> I’m not a fan of implicit contracts just defined by comments.  This method doesn’t seem to be performance critical so I would suggest to use a switch-case.  An attribute on the enum would be much better but we all know this isn’t Java.
> 
> As you suggested:
> http://cr.openjdk.java.net/~neliasso/8153013/webrev.04 <http://cr.openjdk.java.net/~neliasso/8153013/webrev.04>

Thanks.  A space is missing and the closing } indent is wrong:
+   bool         can_become_stale() const          {
+     switch(_compile_reason) {
+       case Reason_BackedgeCount:
+       case Reason_InvocationCount:
+       case Reason_Tiered:
+         return !_is_blocking;
+       }
+     return false;
+   }
Also, what about:
+       Reason_None,
+       Reason_CTW,              // Compile the world
+       Reason_Replay,           // ciReplay
These were covered before.

> 
> Also made reasons CTW and Replay not stale-able. 
> 
> Thanks!
> Nils
> 
>> 
>>> 
>>> Thanks!
>>> Nils
>>> 
>>> On 2016-04-13 23:34, Vladimir Kozlov wrote:
>>>> Very nice, I like it.
>>>> 
>>>> One note. CompileReason (and its names) should be CompileTask class where it is recorded. Then CompileTask::can_become_stale() can be in header file so it is inlinined on all platforms.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 4/13/16 5:59 AM, Nils Eliasson wrote:
>>>>> Hi,
>>>>> 
>>>>> New webrev:
>>>>> http://cr.openjdk.java.net/~neliasso/8153013/webrev.02/ <http://cr.openjdk.java.net/%7Eneliasso/8153013/webrev.02/>
>>>>> 
>>>>> Summary
>>>>> Introduced an enum CompileReason with members matching all the old
>>>>> variants, and a table containing all the unchanged strings. I see the
>>>>> possibility of removing/changing/simplifying some CompileReasons but
>>>>> have choosen not to do so in this change.
>>>>> 
>>>>> Only new logic is the CompileTask::can_become_stale() method.
>>>>> 
>>>>> Testing:
>>>>> Running Testset hotspot on all platforms and hotspot_all on one platform
>>>>> 
>>>>> Regards,
>>>>> Nils Eliawsson
>>>>> 
>>>>> On 2016-04-12 18:55, Vladimir Kozlov wrote:
>>>>>> On 4/12/16 6:30 AM, Nils Eliasson wrote:
>>>>>>> Tasks get evicted from the compile_queue if their invocation counter
>>>>>>> hasn't increased during TieredCompileTaskTimeout.
>>>>>>> (AdvancedThresholdPolicy::is_stale(...)).
>>>>>>> 
>>>>>>> I'll do a proper fix, it is the right thing to do and should be pretty
>>>>>>> quick. I'll change the comment to an enum that represent who submitted
>>>>>>> the compile, and add a table for the comments. This could be useful in
>>>>>>> other settings to.
>>>>>> 
>>>>>> Sounds good.
>>>>>> 
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Nils
>>>>>>> 
>>>>>>> On 2016-04-08 19:09, Vladimir Kozlov wrote:
>>>>>>>> What do you mean "stale"?
>>>>>>>> I would prefer to see the real fix as you suggested to avoid removing
>>>>>>>> WB comp tasks from queue. Adding timeout is not reliable.
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 4/8/16 5:27 AM, Nils Eliasson wrote:
>>>>>>>>> Hi,
>>>>>>>>> 
>>>>>>>>> Please review this small fix of the BlockingCompilation test.
>>>>>>>>> 
>>>>>>>>> Summary:
>>>>>>>>> Add method enqueued for compilation with WB API may be removed from
>>>>>>>>> the compile queue as stale.
>>>>>>>>> 
>>>>>>>>> Solution:
>>>>>>>>> Add -XX:TieredCompileTaskTimeout=60000 to make sure nothing gets
>>>>>>>>> stale while the test is running. (Also added some extra
>>>>>>>>> checks that may spare us from waiting until timeout for failing.)
>>>>>>>>> 
>>>>>>>>> This is an workaround but we should consider fixing something
>>>>>>>>> permanent for WB API compiles - like tagging the compile
>>>>>>>>> task with info about the origin of the compile. The comment field has
>>>>>>>>> this information - but then it needs to be
>>>>>>>>> converted to an enum.
>>>>>>>>> 
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153013 <https://bugs.openjdk.java.net/browse/JDK-8153013>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~neliasso/8153013/webrev.01/ <http://cr.openjdk.java.net/~neliasso/8153013/webrev.01/>
>>>>>>>>> 
>>>>>>>>> Best regards,
>>>>>>>>> Nils Eliasson
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>> 
>>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160415/8770e5ea/attachment.html>