RFR(S): 8153013: BlockingCompilation test times out

Christian Thalinger christian.thalinger at oracle.com
Wed Apr 20 21:47:52 UTC 2016 

Looks good.

> On Apr 19, 2016, at 9:46 PM, Nils Eliasson <nils.eliasson at oracle.com> wrote:
> 
> 
> 
> On 2016-04-19 19:37, Christian Thalinger wrote:
>> 
>>> On Apr 19, 2016, at 7:13 AM, Nils Eliasson < <mailto:nils.eliasson at oracle.com>nils.eliasson at oracle.com <mailto:nils.eliasson at oracle.com>> wrote:
>>> 
>>> 
>>> 
>>> On 2016-04-18 12:24, Nils Eliasson wrote:
>>>> Hi,
>>>> 
>>>> On 2016-04-15 22:43, Christian Thalinger wrote:
>>>>> 
>>>>>> On Apr 15, 2016, at 3:06 AM, Nils Eliasson <nils.eliasson at oracle.com <mailto: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/%7Eneliasso/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;
>>>>> +   }
>>> And I fixed the indentation.
>>> 
>>> Webrev: http://cr.openjdk.java.net/~neliasso/8153013/webrev.05/ <http://cr.openjdk.java.net/%7Eneliasso/8153013/webrev.05/>
>> 
>> +     switch(_compile_reason) {
>> Space after switch.
> 
> New webrev: http://cr.openjdk.java.net/~neliasso/8153013/webrev.06/ <http://cr.openjdk.java.net/~neliasso/8153013/webrev.06/>
> 
> Thanks,
> Nils
> 
>> 
>>> 
>>> Thanks!
>>> Nils
>>>>> Also, what about:
>>>>> +       Reason_None,
>>>>> +       Reason_CTW,              // Compile the world
>>>>> +       Reason_Replay,           // ciReplay
>>>>> These were covered before.
>>>> Reason_None - is only used for bounds checking together with Reason_Count.
>>>> Reason_Replay - if these compilations can get stale we can get indeterminism in replay.
>>>> Reason_CTW - CTW could silently drop compiles -> more indeterminism.
>>>> 
>>>> Regards,
>>>> Nils
>>>> 
>>>>> 
>>>>>> 
>>>>>> 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 <https://bugs.openjdk.java.net/browse/JDK-8153013>
>>>>>>>>>>>>>> Webrev:  <http://cr.openjdk.java.net/%7Eneliasso/8153013/webrev.01/>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/20160420/f2e3735a/attachment-0001.html>

More information about the hotspot-compiler-dev mailing list