RFR(S): 8153013: BlockingCompilation test times out
Christian Thalinger
christian.thalinger at oracle.com
Tue Apr 19 17:37:09 UTC 2016
> On Apr 19, 2016, at 7:13 AM, Nils Eliasson <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 <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/~neliasso/8153013/webrev.05/>
+ switch(_compile_reason) {
Space after switch.
>
> 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>
>>>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~neliasso/8153013/webrev.01/ <http://cr.openjdk.java.net/%7Eneliasso/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/8e8d3d6f/attachment.html>
More information about the hotspot-compiler-dev
mailing list