[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be install
From: Dean Long <dean.long () oracle ! com>
Date: 2020-05-28 17:54:56
Message-ID: 5d957cae-8911-8572-2b45-048b8d09ae79 () oracle ! com
[Download RAW message or body]
Sure, you could just have cache_jvmti_state() return a boolean to bail
out immediately for is_old.
dl
On 5/28/20 7:23 AM, serguei.spitsyn@oracle.com wrote:
> Hi Dean,
>
> Thank you for looking at this!
> Okay. Let me check what cab be done in this direction.
> There is no point to cache is_old. The compilation has to bail out if
> it is discovered to be true.
>
> Thanks,
> Serguei
>
>
> On 5/28/20 00:59, Dean Long wrote:
>> This seems OK as long as the memory barriers in the thread state
>> transitions prevent the C++ compiler from doing something like
>> reading is_old before reading redefinition_count. I would feel
>> better if both JVMCI and C1/C2 cached is_old and redefinition_count
>> at the same time (making sure to be in the _thread_in_vm state), then
>> bail out based on the cached value of is_old.
>>
>> dl
>>
>> On 5/26/20 12:04 AM, serguei.spitsyn@oracle.com wrote:
>>> On 5/25/20 23:39, serguei.spitsyn@oracle.com wrote:
>>>> Please, review a fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8245126
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
>>>>
>>>>
>>>> Summary:
>>>> The Kitchensink stress test with the Instrumentation module
>>>> enabled does
>>>> a lot of class retransformations in parallel with all other
>>>> stressing.
>>>> It provokes the assert at the compiled code installation time:
>>>> assert(!method->is_old()) failed: Should not be installing old
>>>> methods
>>>>
>>>> The problem is that the CompileBroker::invoke_compiler_on_method
>>>> in C2 version
>>>> (non-JVMCI tiered compilation) is missing the check that exists
>>>> in the JVMCI
>>>> part of implementation:
>>>> 2148 // Skip redefined methods
>>>> 2149 if (target_handle->is_old()) {
>>>> 2150 failure_reason = "redefined method";
>>>> 2151 retry_message = "not retryable";
>>>> 2152 compilable = ciEnv::MethodCompilable_never;
>>>> 2153 } else {
>>>> . . .
>>>> 2168 }
>>>>
>>>> The fix is to add this check.
>>>
>>> Sorry, forgot to explain one thing.
>>> Compiler code has a special mechanism to ensure the JVMTI class
>>> redefinition did
>>> not happen while the method was compiled, so all the assumptions
>>> remain correct.
>>> 2190 // Cache Jvmti state
>>> 2191 ci_env.cache_jvmti_state();
>>> Part of this is a check that the value of
>>> JvmtiExport::redefinition_count() is
>>> cached in ciEnv variable: _jvmti_redefinition_count.
>>> The JvmtiExport::redefinition_count() value change means a class
>>> redefinition
>>> happened which also implies some of methods may become old.
>>> However, the method being compiled can be already old at the point
>>> where the
>>> redefinition counter is cached, so the redefinition counter check
>>> does not help much.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> Testing:
>>>> Ran Kitchensink test with the Instrumentation module enabled in mach5
>>>> multiple times for 100 times. Without the fix the test normally fails
>>>> a couple of times in 200 runs. It does not fail with the fix anymore.
>>>> Will also submit hs tiers1-5.
>>>>
>>>> Thanks,
>>>> Serguei
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Sure, you could just have cache_jvmti_state() return a boolean to
bail out immediately for is_old.<br>
<br>
dl<br>
<br>
<div class="moz-cite-prefix">On 5/28/20 7:23 AM,
<a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote type="cite"
cite="mid:e7d84a34-1603-dd81-93a6-8dc19ffbc009@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Dean,<br>
<br>
Thank you for looking at this!<br>
Okay. Let me check what cab be done in this direction.<br>
There is no point to cache is_old. The compilation has to bail
out if it is discovered to be true.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 5/28/20 00:59, Dean Long wrote:<br>
</div>
<blockquote type="cite"
cite="mid:fadcbadb-7699-f5f2-f3f8-adae3e62b8aa@oracle.com"> This
seems OK as long as the memory barriers in the thread state
transitions prevent the C++ compiler from doing something like
reading is_old before reading redefinition_count. I would feel
better if both JVMCI and C1/C2 cached is_old and
redefinition_count at the same time (making sure to be in the
_thread_in_vm state), then bail out based on the cached value of
is_old.<br>
<br>
dl<br>
<br>
<div class="moz-cite-prefix">On 5/26/20 12:04 AM, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:62e34586-eaac-3200-8f5a-ee12ad654afa@oracle.com">
<div class="moz-cite-prefix">On 5/25/20 23:39, <a
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:f8a67bab-b900-fdee-ecf1-f11f751378bc@oracle.com">
Please, review a fix for:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245126"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245126</a><br>
<br>
Webrev:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/</a><br>
<br>
<br>
Summary:<br>
The Kitchensink stress test with the Instrumentation
module enabled does<br>
a lot of class retransformations in parallel with all
other stressing.<br>
It provokes the assert at the compiled code installation
time:<br>
assert(!method->is_old()) failed: Should not be
installing old methods<br>
<br>
The problem is that the
CompileBroker::invoke_compiler_on_method in C2 version<br>
(non-JVMCI tiered compilation) is missing the check that
exists in the JVMCI<br>
part of implementation:<br>
<pre>2148 // Skip redefined methods
2149 if (target_handle->is_old()) {
2150 failure_reason = "redefined method";
2151 retry_message = "not retryable";
2152 compilable = ciEnv::MethodCompilable_never;
2153 } else {
. . .
2168 }
The fix is to add this check.</pre>
</blockquote>
<br>
Sorry, forgot to explain one thing.<br>
Compiler code has a special mechanism to ensure the JVMTI
class redefinition did<br>
not happen while the method was compiled, so all the
assumptions remain correct.<br>
<pre> 2190 // Cache Jvmti state
2191 ci_env.cache_jvmti_state();</pre>
Part of this is a check that the value of
JvmtiExport::redefinition_count() is<br>
cached in ciEnv variable: _jvmti_redefinition_count.<br>
The JvmtiExport::redefinition_count() value change means a
class redefinition<br>
happened which also implies some of methods may become old.<br>
However, the method being compiled can be already old at the
point where the<br>
redefinition counter is cached, so the redefinition counter
check does not help much.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:f8a67bab-b900-fdee-ecf1-f11f751378bc@oracle.com">
<pre>Testing:
Ran Kitchensink test with the Instrumentation module enabled in mach5
multiple times for 100 times. Without the fix the test normally fails
a couple of times in 200 runs. It does not fail with the fix anymore.
Will also submit hs tiers1-5.
Thanks,
Serguei
</pre>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic