[9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8173338

The fix for JDK-7177745 disabled deoptimization count update when it
happens due to CallSite.target change.

There was one case missed though: the count is still updated when
compilation fails due to a dependency becoming invalid during the
compilation.

The fix is to avoid the update when recompilation fails due to
call_site_target dependency failure.

Also, added a check in CodeCache::make_marked_nmethods_not_entrant() to
avoid repeated nmethod::make_not_entrant() calls on nmethods which are
already not entrant. Otherwise, the check is performed under
Patching_lock inside nmethod::make_not_entrant_or_zombie() for every
non-entrant method in the code cache.

Testing: regression test, RBT (in progress)

Thanks!

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Kozlov
It will not work for AOT nmethods which have not_used state.

Vladimir

On 1/26/17 8:47 AM, Vladimir Ivanov wrote:

> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8173338
>
> The fix for JDK-7177745 disabled deoptimization count update when it happens due to CallSite.target change.
>
> There was one case missed though: the count is still updated when compilation fails due to a dependency becoming invalid
> during the compilation.
>
> The fix is to avoid the update when recompilation fails due to call_site_target dependency failure.
>
> Also, added a check in CodeCache::make_marked_nmethods_not_entrant() to avoid repeated nmethod::make_not_entrant() calls
> on nmethods which are already not entrant. Otherwise, the check is performed under Patching_lock inside
> nmethod::make_not_entrant_or_zombie() for every non-entrant method in the code cache.
>
> Testing: regression test, RBT (in progress)
>
> Thanks!
>
> Best regards,
> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
What about moving the check to nmethod::make_not_entrant()?

http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01

Or I can fix it separately.

Best regards,
Vladimir Ivanov

On 1/26/17 11:30 PM, Vladimir Kozlov wrote:

> It will not work for AOT nmethods which have not_used state.
>
> Vladimir
>
> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>
>> The fix for JDK-7177745 disabled deoptimization count update when it
>> happens due to CallSite.target change.
>>
>> There was one case missed though: the count is still updated when
>> compilation fails due to a dependency becoming invalid
>> during the compilation.
>>
>> The fix is to avoid the update when recompilation fails due to
>> call_site_target dependency failure.
>>
>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>> to avoid repeated nmethod::make_not_entrant() calls
>> on nmethods which are already not entrant. Otherwise, the check is
>> performed under Patching_lock inside
>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>> the code cache.
>>
>> Testing: regression test, RBT (in progress)
>>
>> Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

John Rose-3
On Jan 26, 2017, at 12:44 PM, Vladimir Ivanov <[hidden email]> wrote:
>
> What about moving the check to nmethod::make_not_entrant()?
>
> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01

Looks good.

Suggest renaming _failure_inc_decompile_count to _inc_decompile_count_on_failure
so you can just remove the explanatory comment in ciEnv.hpp.

I prefer the .01 version with the pre-lock check in make_not_entrant.

Actually it would be even cleaner to put the pre-lock check in
the same method that uses the lock, make_not_entrant_or_zombie.

diff --git a/src/share/vm/code/nmethod.cpp b/src/share/vm/code/nmethod.cpp
--- a/src/share/vm/code/nmethod.cpp
+++ b/src/share/vm/code/nmethod.cpp
@@ -1146,6 +1146,14 @@
   assert(state == zombie || state == not_entrant, "must be zombie or not_entrant");
   assert(!is_zombie(), "should not already be a zombie");
 
+  if (_state == state) {
+    // Avoid taking the lock if already in required state.
+    // This is safe from races because the state is an end-state,
+    // which the nmethod cannot back out of once entered.
+    // No need for fencing either.
+    return false;
+  }
+
   // Make sure neither the nmethod nor the method is flushed in case of a safepoint in code below.
   nmethodLocker nml(this);
   methodHandle the_method(method());

Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Kozlov
In reply to this post by Vladimir Ivanov
It could be unsafe to check outside lock. method::_code is cleaned up under the same lock but after the state change. So
if a caller access _code field it would be wrong.

But, I think, it should be safe with your original change in CodeCache::make_marked_nmethods_not_entrant() because it
does not access method::_code. You should use !is_not_entrant() instead of is_alive() - it works for AOT too.

Thanks,
Vladimir K

On 1/26/17 12:44 PM, Vladimir Ivanov wrote:

> What about moving the check to nmethod::make_not_entrant()?
>
> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>
> Or I can fix it separately.
>
> Best regards,
> Vladimir Ivanov
>
> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>> It will not work for AOT nmethods which have not_used state.
>>
>> Vladimir
>>
>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>
>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>> happens due to CallSite.target change.
>>>
>>> There was one case missed though: the count is still updated when
>>> compilation fails due to a dependency becoming invalid
>>> during the compilation.
>>>
>>> The fix is to avoid the update when recompilation fails due to
>>> call_site_target dependency failure.
>>>
>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>> to avoid repeated nmethod::make_not_entrant() calls
>>> on nmethods which are already not entrant. Otherwise, the check is
>>> performed under Patching_lock inside
>>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>>> the code cache.
>>>
>>> Testing: regression test, RBT (in progress)
>>>
>>> Thanks!
>>>
>>> Best regards,
>>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Kozlov
On 1/26/17 3:06 PM, Vladimir Kozlov wrote:
> It could be unsafe to check outside lock. method::_code is cleaned up under the same lock but after the state change. So
> if a caller access _code field it would be wrong.
>
> But, I think, it should be safe with your original change in CodeCache::make_marked_nmethods_not_entrant() because it
> does not access method::_code. You should use !is_not_entrant() instead of is_alive() - it works for AOT too.

"instead of is_in_use()"

>
> Thanks,
> Vladimir K
>
> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>> What about moving the check to nmethod::make_not_entrant()?
>>
>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>
>> Or I can fix it separately.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>> It will not work for AOT nmethods which have not_used state.
>>>
>>> Vladimir
>>>
>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>
>>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>>> happens due to CallSite.target change.
>>>>
>>>> There was one case missed though: the count is still updated when
>>>> compilation fails due to a dependency becoming invalid
>>>> during the compilation.
>>>>
>>>> The fix is to avoid the update when recompilation fails due to
>>>> call_site_target dependency failure.
>>>>
>>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>> performed under Patching_lock inside
>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>>>> the code cache.
>>>>
>>>> Testing: regression test, RBT (in progress)
>>>>
>>>> Thanks!
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
In reply to this post by Vladimir Kozlov
Vladimir, John, thanks for the feedback.

I incorporated your suggestions:

   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.02/

> It could be unsafe to check outside lock. method::_code is cleaned up
> under the same lock but after the state change. So if a caller access
> _code field it would be wrong.

I don't see how method::_code is relevant to compiled method state
checks. It's just a double-checked locking idiom. Should be safe since
state transitions are monotonic.

> But, I think, it should be safe with your original change in
> CodeCache::make_marked_nmethods_not_entrant() because it does not access
> method::_code. You should use !is_not_entrant() instead of is_alive() -
> it works for AOT too.

I decided to keep state checks both in
CodeCache::make_marked_nmethods_not_entrant() and
nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
enough, since it passes for zombie nmethods as well.

Also, added equivalent pre-lock check in
AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see any
problems with that.

Best regards,
Vladimir Ivanov

> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>> What about moving the check to nmethod::make_not_entrant()?
>>
>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>
>> Or I can fix it separately.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>> It will not work for AOT nmethods which have not_used state.
>>>
>>> Vladimir
>>>
>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>
>>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>>> happens due to CallSite.target change.
>>>>
>>>> There was one case missed though: the count is still updated when
>>>> compilation fails due to a dependency becoming invalid
>>>> during the compilation.
>>>>
>>>> The fix is to avoid the update when recompilation fails due to
>>>> call_site_target dependency failure.
>>>>
>>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>> performed under Patching_lock inside
>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>>>> the code cache.
>>>>
>>>> Testing: regression test, RBT (in progress)
>>>>
>>>> Thanks!
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Dean Long
On 1/27/17 4:29 AM, Vladimir Ivanov wrote:

> Vladimir, John, thanks for the feedback.
>
> I incorporated your suggestions:
>
>   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.02/
>
>> It could be unsafe to check outside lock. method::_code is cleaned up
>> under the same lock but after the state change. So if a caller access
>> _code field it would be wrong.
>
> I don't see how method::_code is relevant to compiled method state
> checks. It's just a double-checked locking idiom. Should be safe since
> state transitions are monotonic.
>

I think there is a path where AOT methods can become in-use again, so
it's not completely monotonic.

dl

>> But, I think, it should be safe with your original change in
>> CodeCache::make_marked_nmethods_not_entrant() because it does not access
>> method::_code. You should use !is_not_entrant() instead of is_alive() -
>> it works for AOT too.
>
> I decided to keep state checks both in
> CodeCache::make_marked_nmethods_not_entrant() and
> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
> enough, since it passes for zombie nmethods as well.
>
> Also, added equivalent pre-lock check in
> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
> any problems with that.
>
> Best regards,
> Vladimir Ivanov
>
>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>> What about moving the check to nmethod::make_not_entrant()?
>>>
>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>
>>> Or I can fix it separately.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>> It will not work for AOT nmethods which have not_used state.
>>>>
>>>> Vladimir
>>>>
>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>
>>>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>>>> happens due to CallSite.target change.
>>>>>
>>>>> There was one case missed though: the count is still updated when
>>>>> compilation fails due to a dependency becoming invalid
>>>>> during the compilation.
>>>>>
>>>>> The fix is to avoid the update when recompilation fails due to
>>>>> call_site_target dependency failure.
>>>>>
>>>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>>> performed under Patching_lock inside
>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>>>>> the code cache.
>>>>>
>>>>> Testing: regression test, RBT (in progress)
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov

Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
>> I don't see how method::_code is relevant to compiled method state
>> checks. It's just a double-checked locking idiom. Should be safe since
>> state transitions are monotonic.
>>
>
> I think there is a path where AOT methods can become in-use again, so
> it's not completely monotonic.

Ok, I can leave the pre-lock check only for nmethods.

Best regards,
Vladimir Ivanov

>>> But, I think, it should be safe with your original change in
>>> CodeCache::make_marked_nmethods_not_entrant() because it does not access
>>> method::_code. You should use !is_not_entrant() instead of is_alive() -
>>> it works for AOT too.
>>
>> I decided to keep state checks both in
>> CodeCache::make_marked_nmethods_not_entrant() and
>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
>> enough, since it passes for zombie nmethods as well.
>>
>> Also, added equivalent pre-lock check in
>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>> any problems with that.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>
>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>
>>>> Or I can fix it separately.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>
>>>>> Vladimir
>>>>>
>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>
>>>>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>>>>> happens due to CallSite.target change.
>>>>>>
>>>>>> There was one case missed though: the count is still updated when
>>>>>> compilation fails due to a dependency becoming invalid
>>>>>> during the compilation.
>>>>>>
>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>> call_site_target dependency failure.
>>>>>>
>>>>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>>>> performed under Patching_lock inside
>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant method in
>>>>>> the code cache.
>>>>>>
>>>>>> Testing: regression test, RBT (in progress)
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
Are people fine with the following version?

   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.03/

Otherwise, I'll file a separate RFE for
CodeCache::make_marked_nmethods_not_entrant() part.

Best regards,
Vladimir Ivanov

On 1/27/17 11:11 PM, Vladimir Ivanov wrote:

>>> I don't see how method::_code is relevant to compiled method state
>>> checks. It's just a double-checked locking idiom. Should be safe since
>>> state transitions are monotonic.
>>>
>>
>> I think there is a path where AOT methods can become in-use again, so
>> it's not completely monotonic.
>
> Ok, I can leave the pre-lock check only for nmethods.
>
> Best regards,
> Vladimir Ivanov
>
>>>> But, I think, it should be safe with your original change in
>>>> CodeCache::make_marked_nmethods_not_entrant() because it does not
>>>> access
>>>> method::_code. You should use !is_not_entrant() instead of is_alive() -
>>>> it works for AOT too.
>>>
>>> I decided to keep state checks both in
>>> CodeCache::make_marked_nmethods_not_entrant() and
>>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
>>> enough, since it passes for zombie nmethods as well.
>>>
>>> Also, added equivalent pre-lock check in
>>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>>> any problems with that.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>>
>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>>
>>>>> Or I can fix it separately.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>>
>>>>>>> The fix for JDK-7177745 disabled deoptimization count update when it
>>>>>>> happens due to CallSite.target change.
>>>>>>>
>>>>>>> There was one case missed though: the count is still updated when
>>>>>>> compilation fails due to a dependency becoming invalid
>>>>>>> during the compilation.
>>>>>>>
>>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>>> call_site_target dependency failure.
>>>>>>>
>>>>>>> Also, added a check in CodeCache::make_marked_nmethods_not_entrant()
>>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>>>>> performed under Patching_lock inside
>>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant
>>>>>>> method in
>>>>>>> the code cache.
>>>>>>>
>>>>>>> Testing: regression test, RBT (in progress)
>>>>>>>
>>>>>>> Thanks!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Dean Long
Are there any other uses of next_alive() where we would want something
more strict like next_entrant()?  If so then it might be worth it to add
this to the iterator.

dl


On 1/30/17 5:10 AM, Vladimir Ivanov wrote:

> Are people fine with the following version?
>
>   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.03/
>
> Otherwise, I'll file a separate RFE for
> CodeCache::make_marked_nmethods_not_entrant() part.
>
> Best regards,
> Vladimir Ivanov
>
> On 1/27/17 11:11 PM, Vladimir Ivanov wrote:
>>>> I don't see how method::_code is relevant to compiled method state
>>>> checks. It's just a double-checked locking idiom. Should be safe since
>>>> state transitions are monotonic.
>>>>
>>>
>>> I think there is a path where AOT methods can become in-use again, so
>>> it's not completely monotonic.
>>
>> Ok, I can leave the pre-lock check only for nmethods.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>>>> But, I think, it should be safe with your original change in
>>>>> CodeCache::make_marked_nmethods_not_entrant() because it does not
>>>>> access
>>>>> method::_code. You should use !is_not_entrant() instead of
>>>>> is_alive() -
>>>>> it works for AOT too.
>>>>
>>>> I decided to keep state checks both in
>>>> CodeCache::make_marked_nmethods_not_entrant() and
>>>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
>>>> enough, since it passes for zombie nmethods as well.
>>>>
>>>> Also, added equivalent pre-lock check in
>>>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>>>> any problems with that.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>>>
>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>>>
>>>>>> Or I can fix it separately.
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>>>
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>>>
>>>>>>>> The fix for JDK-7177745 disabled deoptimization count update
>>>>>>>> when it
>>>>>>>> happens due to CallSite.target change.
>>>>>>>>
>>>>>>>> There was one case missed though: the count is still updated when
>>>>>>>> compilation fails due to a dependency becoming invalid
>>>>>>>> during the compilation.
>>>>>>>>
>>>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>>>> call_site_target dependency failure.
>>>>>>>>
>>>>>>>> Also, added a check in
>>>>>>>> CodeCache::make_marked_nmethods_not_entrant()
>>>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>>>>>> performed under Patching_lock inside
>>>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant
>>>>>>>> method in
>>>>>>>> the code cache.
>>>>>>>>
>>>>>>>> Testing: regression test, RBT (in progress)
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>

Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
Dean,

> Are there any other uses of next_alive() where we would want something
> more strict like next_entrant()?  If so then it might be worth it to add
> this to the iterator.

I briefly looked through next_alive() usages and didn't find evident
candidates.

Also, is_marked_for_deoptimization() is cheap and filters out most of
the compiled methods, so doing it first may be better.

Best regards,
Vladimir Ivanov

> On 1/30/17 5:10 AM, Vladimir Ivanov wrote:
>> Are people fine with the following version?
>>
>>   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.03/
>>
>> Otherwise, I'll file a separate RFE for
>> CodeCache::make_marked_nmethods_not_entrant() part.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> On 1/27/17 11:11 PM, Vladimir Ivanov wrote:
>>>>> I don't see how method::_code is relevant to compiled method state
>>>>> checks. It's just a double-checked locking idiom. Should be safe since
>>>>> state transitions are monotonic.
>>>>>
>>>>
>>>> I think there is a path where AOT methods can become in-use again, so
>>>> it's not completely monotonic.
>>>
>>> Ok, I can leave the pre-lock check only for nmethods.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>>>> But, I think, it should be safe with your original change in
>>>>>> CodeCache::make_marked_nmethods_not_entrant() because it does not
>>>>>> access
>>>>>> method::_code. You should use !is_not_entrant() instead of
>>>>>> is_alive() -
>>>>>> it works for AOT too.
>>>>>
>>>>> I decided to keep state checks both in
>>>>> CodeCache::make_marked_nmethods_not_entrant() and
>>>>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't precise
>>>>> enough, since it passes for zombie nmethods as well.
>>>>>
>>>>> Also, added equivalent pre-lock check in
>>>>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>>>>> any problems with that.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>>>>
>>>>>>> Or I can fix it separately.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>>>>
>>>>>>>>> The fix for JDK-7177745 disabled deoptimization count update
>>>>>>>>> when it
>>>>>>>>> happens due to CallSite.target change.
>>>>>>>>>
>>>>>>>>> There was one case missed though: the count is still updated when
>>>>>>>>> compilation fails due to a dependency becoming invalid
>>>>>>>>> during the compilation.
>>>>>>>>>
>>>>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>>>>> call_site_target dependency failure.
>>>>>>>>>
>>>>>>>>> Also, added a check in
>>>>>>>>> CodeCache::make_marked_nmethods_not_entrant()
>>>>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>>>>> on nmethods which are already not entrant. Otherwise, the check is
>>>>>>>>> performed under Patching_lock inside
>>>>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant
>>>>>>>>> method in
>>>>>>>>> the code cache.
>>>>>>>>>
>>>>>>>>> Testing: regression test, RBT (in progress)
>>>>>>>>>
>>>>>>>>> Thanks!
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Dean Long
On 1/30/17 1:06 PM, Vladimir Ivanov wrote:

> Dean,
>
>> Are there any other uses of next_alive() where we would want something
>> more strict like next_entrant()?  If so then it might be worth it to add
>> this to the iterator.
>
> I briefly looked through next_alive() usages and didn't find evident
> candidates.
>
> Also, is_marked_for_deoptimization() is cheap and filters out most of
> the compiled methods, so doing it first may be better.
>

OK, then webrev.03 looks good to me.

dl

> Best regards,
> Vladimir Ivanov
>
>> On 1/30/17 5:10 AM, Vladimir Ivanov wrote:
>>> Are people fine with the following version?
>>>
>>>   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.03/
>>>
>>> Otherwise, I'll file a separate RFE for
>>> CodeCache::make_marked_nmethods_not_entrant() part.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 1/27/17 11:11 PM, Vladimir Ivanov wrote:
>>>>>> I don't see how method::_code is relevant to compiled method state
>>>>>> checks. It's just a double-checked locking idiom. Should be safe
>>>>>> since
>>>>>> state transitions are monotonic.
>>>>>>
>>>>>
>>>>> I think there is a path where AOT methods can become in-use again, so
>>>>> it's not completely monotonic.
>>>>
>>>> Ok, I can leave the pre-lock check only for nmethods.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>>>>> But, I think, it should be safe with your original change in
>>>>>>> CodeCache::make_marked_nmethods_not_entrant() because it does not
>>>>>>> access
>>>>>>> method::_code. You should use !is_not_entrant() instead of
>>>>>>> is_alive() -
>>>>>>> it works for AOT too.
>>>>>>
>>>>>> I decided to keep state checks both in
>>>>>> CodeCache::make_marked_nmethods_not_entrant() and
>>>>>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't
>>>>>> precise
>>>>>> enough, since it passes for zombie nmethods as well.
>>>>>>
>>>>>> Also, added equivalent pre-lock check in
>>>>>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>>>>>> any problems with that.
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir Ivanov
>>>>>>
>>>>>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>>>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>>>>>
>>>>>>>> Or I can fix it separately.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Vladimir Ivanov
>>>>>>>>
>>>>>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>>>>>
>>>>>>>>> Vladimir
>>>>>>>>>
>>>>>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>>>>>
>>>>>>>>>> The fix for JDK-7177745 disabled deoptimization count update
>>>>>>>>>> when it
>>>>>>>>>> happens due to CallSite.target change.
>>>>>>>>>>
>>>>>>>>>> There was one case missed though: the count is still updated
>>>>>>>>>> when
>>>>>>>>>> compilation fails due to a dependency becoming invalid
>>>>>>>>>> during the compilation.
>>>>>>>>>>
>>>>>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>>>>>> call_site_target dependency failure.
>>>>>>>>>>
>>>>>>>>>> Also, added a check in
>>>>>>>>>> CodeCache::make_marked_nmethods_not_entrant()
>>>>>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>>>>>> on nmethods which are already not entrant. Otherwise, the
>>>>>>>>>> check is
>>>>>>>>>> performed under Patching_lock inside
>>>>>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant
>>>>>>>>>> method in
>>>>>>>>>> the code cache.
>>>>>>>>>>
>>>>>>>>>> Testing: regression test, RBT (in progress)
>>>>>>>>>>
>>>>>>>>>> Thanks!
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> Vladimir Ivanov
>>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

Vladimir Ivanov
Thanks, Dean.

Best regards,
Vladimir Ivanov

On 1/31/17 12:32 AM, [hidden email] wrote:

> On 1/30/17 1:06 PM, Vladimir Ivanov wrote:
>
>> Dean,
>>
>>> Are there any other uses of next_alive() where we would want something
>>> more strict like next_entrant()?  If so then it might be worth it to add
>>> this to the iterator.
>>
>> I briefly looked through next_alive() usages and didn't find evident
>> candidates.
>>
>> Also, is_marked_for_deoptimization() is cheap and filters out most of
>> the compiled methods, so doing it first may be better.
>>
>
> OK, then webrev.03 looks good to me.
>
> dl
>
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 1/30/17 5:10 AM, Vladimir Ivanov wrote:
>>>> Are people fine with the following version?
>>>>
>>>>   http://cr.openjdk.java.net/~vlivanov/8173338/webrev.03/
>>>>
>>>> Otherwise, I'll file a separate RFE for
>>>> CodeCache::make_marked_nmethods_not_entrant() part.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
>>>>
>>>> On 1/27/17 11:11 PM, Vladimir Ivanov wrote:
>>>>>>> I don't see how method::_code is relevant to compiled method state
>>>>>>> checks. It's just a double-checked locking idiom. Should be safe
>>>>>>> since
>>>>>>> state transitions are monotonic.
>>>>>>>
>>>>>>
>>>>>> I think there is a path where AOT methods can become in-use again, so
>>>>>> it's not completely monotonic.
>>>>>
>>>>> Ok, I can leave the pre-lock check only for nmethods.
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>>>>> But, I think, it should be safe with your original change in
>>>>>>>> CodeCache::make_marked_nmethods_not_entrant() because it does not
>>>>>>>> access
>>>>>>>> method::_code. You should use !is_not_entrant() instead of
>>>>>>>> is_alive() -
>>>>>>>> it works for AOT too.
>>>>>>>
>>>>>>> I decided to keep state checks both in
>>>>>>> CodeCache::make_marked_nmethods_not_entrant() and
>>>>>>> nmethod::make_not_entrant_or_zombie(). !is_not_entrant() isn't
>>>>>>> precise
>>>>>>> enough, since it passes for zombie nmethods as well.
>>>>>>>
>>>>>>> Also, added equivalent pre-lock check in
>>>>>>> AOTCompiledMethod::make_not_entrant_helper(). Let me know if you see
>>>>>>> any problems with that.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>>> On 1/26/17 12:44 PM, Vladimir Ivanov wrote:
>>>>>>>>> What about moving the check to nmethod::make_not_entrant()?
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.01
>>>>>>>>>
>>>>>>>>> Or I can fix it separately.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Vladimir Ivanov
>>>>>>>>>
>>>>>>>>> On 1/26/17 11:30 PM, Vladimir Kozlov wrote:
>>>>>>>>>> It will not work for AOT nmethods which have not_used state.
>>>>>>>>>>
>>>>>>>>>> Vladimir
>>>>>>>>>>
>>>>>>>>>> On 1/26/17 8:47 AM, Vladimir Ivanov wrote:
>>>>>>>>>>> http://cr.openjdk.java.net/~vlivanov/8173338/webrev.00/
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8173338
>>>>>>>>>>>
>>>>>>>>>>> The fix for JDK-7177745 disabled deoptimization count update
>>>>>>>>>>> when it
>>>>>>>>>>> happens due to CallSite.target change.
>>>>>>>>>>>
>>>>>>>>>>> There was one case missed though: the count is still updated
>>>>>>>>>>> when
>>>>>>>>>>> compilation fails due to a dependency becoming invalid
>>>>>>>>>>> during the compilation.
>>>>>>>>>>>
>>>>>>>>>>> The fix is to avoid the update when recompilation fails due to
>>>>>>>>>>> call_site_target dependency failure.
>>>>>>>>>>>
>>>>>>>>>>> Also, added a check in
>>>>>>>>>>> CodeCache::make_marked_nmethods_not_entrant()
>>>>>>>>>>> to avoid repeated nmethod::make_not_entrant() calls
>>>>>>>>>>> on nmethods which are already not entrant. Otherwise, the
>>>>>>>>>>> check is
>>>>>>>>>>> performed under Patching_lock inside
>>>>>>>>>>> nmethod::make_not_entrant_or_zombie() for every non-entrant
>>>>>>>>>>> method in
>>>>>>>>>>> the code cache.
>>>>>>>>>>>
>>>>>>>>>>> Testing: regression test, RBT (in progress)
>>>>>>>>>>>
>>>>>>>>>>> Thanks!
>>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Vladimir Ivanov
>>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [9] RFR (XS): C2: continuous CallSite relinkage eventually disables compilation for a method

John Rose-3
In reply to this post by Dean Long
On Jan 30, 2017, at 1:32 PM, [hidden email] wrote:

OK, then webrev.03 looks good to me.

+1