RFR(S): 8153013: BlockingCompilation test times out

[Nils Eliasson]

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> wrote:
>>>
>>> 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> 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
>>
>> 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/

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
>>>>>>>>>>> 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/20160419/5d07c5be/attachment-0001.html>