RFR(S): 8153013: BlockingCompilation test times out

Wed Apr 20 07:46:17 UTC 2016


On 2016-04-19 19:37, Christian Thalinger wrote:
>
>> On Apr 19, 2016, at 7:13 AM, Nils Eliasson <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> 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/
>
> *+ switch(_compile_reason) {*
> Space after switch.

New webrev: 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
>>>>>>>>>>>>> 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/20160420/6b14238b/attachment-0001.html>