RFR(S): 8153013: BlockingCompilation test times out

[Nils Eliasson]

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 
>>>> <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;*
> *+ }*
> 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/20160418/c287b594/attachment-0001.html>