RFR(S): 8153013: BlockingCompilation test times out

[Nils Eliasson]

Hi,

On 2016-04-14 20:45, Christian Thalinger wrote:
>
>> On Apr 14, 2016, at 2:43 AM, Nils Eliasson <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

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
>>>>>>>> Webrev: 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/0cd0a37e/attachment.html>