<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<div class="moz-cite-prefix">Hi Dean,<br>
<br>
To check the is_old as you suggest the target method has to be
passed<br>
to the cache_jvmti_state() as argument. Is it what you are
suggesting?<br>
Just want to make sure I understand you correctly.<br>
<br>
The cache_jvmti_state() and cache_dtrace_flags() are called in the<br>
CompileBroker::init_compiler_runtime() for a ciEnv with the NULL
CompileTask<br>
which looks unnecessary (or I don't understand it):<br>
<br>
bool CompileBroker::init_compiler_runtime() {<br>
 CompilerThread* thread = CompilerThread::current();<br>
 . . .<br>
   ciEnv ci_env((CompileTask*)NULL);<br>
   // Cache Jvmti state<br>
   ci_env.cache_jvmti_state();<br>
   // Cache DTrace flags<br>
   ci_env.cache_dtrace_flags();<br>
<br>
The JVMCI has a separate implementation for ciEnv which is
jvmciEnv and<br>
its own set of cache_jvmti_state() and jvmti_state_changed()
functions.<br>
Both are not called in the JVMCI case.<br>
So, these checks look as broken in JVMCI now.<br>
<br>
Not sure, I have enough compiler knowledge to fix this at this
stage of release.<br>
Would it better to file a separate hotspot/compiler RFE targeted
to 16?<br>
It can be assigned to me if it helps.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 5/28/20 10:54, Dean Long wrote:<br>
</div>
<blockquote type="cite"
cite="mid:5d957cae-8911-8572-2b45-048b8d09ae79@oracle.com"> 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"
moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e7d84a34-1603-dd81-93a6-8dc19ffbc009@oracle.com">
<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>
</blockquote>
<br>
</body>
</html>