<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<tt>Hi Serguei,<br>
<br>
Sorry for the late review...<br>
</tt><br>
<div class="moz-cite-prefix">On 5/28/20 7:16 PM,
<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:3a497901-7a05-e87a-33e6-6f1011c32b8b@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Coleen,<br>
<br>
The updated webrev version is:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.3/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.3/</a><br>
</div>
</blockquote>
<tt> <br>
src/hotspot/share/oops/cpCache.cpp<br>
nit - please update copyright year before you push<br>
<br>
L570: log_trace(redefine, class, update, constantpool)<br>
L571: ("cpc %s entry update: %s", entry_type,
new_method->external_name());<br>
nit - The continued line indent on the other line you
touched in this<br>
file is two spaces. This one is six... Not your
bug, but can you<br>
fix it while you are here?<br>
<br>
L816: ("cpcache check found old method entry: class:
%s, old: %d, obsolete: %d, method: %s\n",<br>
nit - I don't think you want the "\n" here.<br>
<br>
src/hotspot/share/oops/klassVtable.cpp<br>
L1004: ("vtable check found old method entry: class:
%s old: %d obsolete: %d, %s\n",<br>
L1319: ("itable check found old method entry: class:
%s old: %d obsolete: %d, %s\n",<br>
nit - I don't think you want the "\n" here.<br>
<br>
In the new log_trace() call in cpCache.cpp, you include a<br>
label for the method output:<br>
<br>
L816: ("cpcache check found old method entry:
class: %s, old: %d, obsolete: %d, method: %s\n",<br>
<br>
but you don't here. I think you should.<br>
<br>
src/hotspot/share/prims/jvmtiRedefineClasses.cpp<br>
L74: // this flag is global as the constructor does not reset
it<br>
nit - Please s/this/This/ and add a ':' to the end.<br>
<br>
old L3586: if (!_has_null_class_loader &&
ik->class_loader() == NULL) {<br>
old L3587: return;<br>
This optimization has been here for a long time! Thanks
for the<br>
explanation in "3) Optimization based on the flag
_has_null_class_loader"<br>
below... I'm probably the one that got that wrong so long
ago...<br>
<br>
L3601: // and needs cpchache method entries adjusted. For
simplicity, the cpcache<br>
typo - s/cpchache/cpcache/<br>
<br>
old L3616: if (!ik->is_being_redefined()) {<br>
Nice explanation on L3599-3604 for why this optimization
is<br>
not a good idea.<br>
<br>
<br>
Thumbs up! I only have nits and typos above. I don't need to see<br>
another webrev.<br>
<br>
Dan<br>
<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:3a497901-7a05-e87a-33e6-6f1011c32b8b@oracle.com">
<div class="moz-cite-prefix"> <br>
It has your suggestions addressed:<br>
- <span class="new">remove log_is_enabled conditions<br>
- move ResourceMark's out of loops<br>
</span><br>
Thanks,<br>
Serguei<br>
<br>
<span class="new"></span><br>
On 5/28/20 14:44, <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:9b75fa4e-f579-e4a7-7996-bc307d001972@oracle.com">
<div class="moz-cite-prefix">Hi Coleen,<br>
<br>
Thank you a lot for reviewing this!<br>
<br>
<br>
On 5/28/20 12:48, <a class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:31ca58d7-99ac-c53d-461f-680461fb5698@oracle.com"> Hi
Serguei,<br>
Sorry for the delay reviewing this again.<br>
<br>
<div class="moz-cite-prefix">On 5/18/20 3:30 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:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix">Hi Coleen and potential
reviewers,<br>
<br>
Now, the webrev:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/</a><br>
<br>
has a complete fix for all three failure modes related to
the guarantee about OLD and OBSOLETE methods.<br>
<br>
The root cause are the following optimizations:<br>
<br>
1) Optimization based on the flag
ik->is_being_redefined():<br>
The problem is that the cpcache method entries of such
classes are not being adjusted.<br>
It is explained below in the initial RFR summary.<br>
The fix is to get rid of this optimization.<br>
</div>
</blockquote>
<br>
This seems like a good thing to do even though (actually
especially because) I can't re-imagine the logic that went
into this optimization.<br>
</blockquote>
<br>
Probably, I've not explained it well enough.<br>
The logic was that the class marked as is_being_redefined was
considered as being redefined in the current redefinition
operation.<br>
For classes redefined in current redefinition the cpcache is
empty, so there is nothing to adjust.<br>
The problem is that classes can be marked as is_being_redefined
by doit_prologue of one of the following redefinition
operations.<br>
In such a case, the VM_RedefineClasses::CheckClass::do_klass
fails with this guarantee.<br>
It is because the VM_RedefineClasses::CheckClass::do_klass does
not have this optimization<br>
and does not skip such classes as the
VM_RedefineClasses::AdjustAndCleanMetadata::do_class.<br>
Without this catch this issue could have unknown consequences in
the future execution far away from the root cause.<br>
<br>
<blockquote type="cite"
cite="mid:31ca58d7-99ac-c53d-461f-680461fb5698@oracle.com">
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix"> <br>
2) Optimization for array classes based on the flag
_has_redefined_Object.<br>
The problem is that the vtable method entries are not
adjusted for array classes.<br>
The array classes have to be adjusted even if the
java.lang.Object was redefined<br>
by one of previous VM_RedefineClasses operation, not
only if it was redefined in<br>
the current VM_RedefineClasses operation. The fix is
is follow this requirement.<br>
</div>
</blockquote>
<br>
This I can't understand. The redefinitions are serialized in
safepoints, so why would you need to replace vtable entries
for arrays if java.lang.Object isn't redefined in this
safepoint?<br>
</blockquote>
The VM_RedefineClasses::CheckClass::do_klass fails with the same
guarantee because of this.<br>
It never fails this way with this optimization relaxed.<br>
I've already broke my head trying to understand it.<br>
It can be because of another bug we don't know yet.<br>
<br>
<blockquote type="cite"
cite="mid:31ca58d7-99ac-c53d-461f-680461fb5698@oracle.com">
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix"> <br>
3) Optimization based on the flag <span class="removed">_has_null_class_loader
which assumes that the Hotspot<br>
does not support delegation </span><span
class="removed">from the bootstrap class loader to a</span><span
class="removed"> user-defined class<br>
loader.</span><span class="removed"> The assumption
is that if the current class being redefined has a
user-defined<br>
class</span><span class="removed"> loader as its
defining class loader, then all</span><span
class="removed"> classes loaded by the bootstrap<br>
class loader can be skipped for vtable/itable method
entries adjustment.<br>
The problem is that this assumption is not really
correct. There are classes that<br>
still need the adjustment. For instance, the class
java.util.IdentityHashMap$KeyIterator<br>
loaded by the bootstrap class loader has the
vtable/itable references to the method:<br>
</span><span class="removed"><span
class="removed"></span>java.util.Iterator.forEachRemaining(java.util.function.Consumer)<br>
The class </span><span class="removed"></span><span
class="removed"><span class="removed"></span>java.util.Iterator
is defined by a user-defined class loader.<br>
The fix is to get rid of this optimization.<br>
</span></div>
</blockquote>
<br>
Also with this optimization, I'm not sure what the logic was
that determined that this was safe, so it's best to remove
it. Above makes sense.<br>
</blockquote>
<br>
I don't know the full theory behind this optimization. We only
have a comment.<br>
<br>
<span class="removed"></span><br>
<blockquote type="cite"
cite="mid:31ca58d7-99ac-c53d-461f-680461fb5698@oracle.com">
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com"><span
class="removed"></span>
<div class="moz-cite-prefix"><span class="removed"> All
three failure modes are observed with the -Xcomp flag.<br>
With all three fixes above in place, the Kitchensink
does not fail with this guarantee anymore.<br>
</span></div>
</blockquote>
<br>
<br>
<a
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/cpCache.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/cpCache.cpp.udiff.html</a><br>
<br>
For logging, the log_trace function will also repeat the 'if'
statement and not allocate the external_name() if logging
isn't specified, so you don't need the 'if' statement above.<br>
<br>
<pre><span class="new">+ if (log_is_enabled(Trace, redefine, class, update)) {</span>
<span class="new">+ log_trace(redefine, class, update, constantpool)</span>
<span class="new">+ ("cpc %s entry update: %s", entry_type, new_method->external_name());
</span><span class="new"></span>
</pre>
<a
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/klassVtable.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/src/hotspot/share/oops/klassVtable.cpp.udiff.html</a><br>
<br>
Same in two cases here, and you could move the ResourceMark
outside the loop at the top.<br>
</blockquote>
<br>
Good suggestions, taken.<br>
<br>
Thanks!<br>
Serguei<br>
<br>
<blockquote type="cite"
cite="mid:31ca58d7-99ac-c53d-461f-680461fb5698@oracle.com"> <br>
Thanks,<br>
Coleen<br>
<pre><span class="new"></span></pre>
<blockquote type="cite"
cite="mid:f97d9630-df42-e874-46f6-e33ac34fad1a@oracle.com">
<div class="moz-cite-prefix"><span class="removed"> </span><span
class="removed"></span><span class="removed"></span><span
class="removed"></span><br>
There is still a JIT compiler relted failure:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245128"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245128</a><br>
Kitchensink fails with: assert(destination ==
(address)-1 || destination == entry) failed: b) MT-unsafe
modification of inline cache<br>
<br>
I also saw this failure but just once:<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>
Kitchensink fails with: assert(!method->is_old())
failed: Should not be installing old methods<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 5/15/20 15:14, <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:52ba0f0f-a705-2043-1c1d-15ba4a441aba@oracle.com">
<div class="moz-cite-prefix">Hi Coleen,<br>
<br>
Thanks a lot for review!<br>
Good suggestion, will use it.<br>
<br>
In fact, I've found two more related problems with the
same guarantee.<br>
One is with vtable method entries adjustment and another
with itable.<br>
This webrev version includes a fix for the vtable
related issue:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.2/</a><br>
<br>
I'm still investigating the itable related issue.<br>
<br>
It is interesting that the Kitchensink with
Instrumentation modules enabled is like a Pandora box
full of surprises.<br>
New problems are getting discovered after some road
blocks are removed.<br>
I've just filed a couple of compiler bugs discovered in
this mode of testing:<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>
Kitchensink fails with: assert(!method->is_old())
failed: Should not be installing old methods<br>
<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8245128"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8245128</a><br>
Kitchensink fails with: assert(destination ==
(address)-1 || destination == entry) failed: b)
MT-unsafe modification of inline cache<br>
<br>
<br>
Thanks,<br>
Serguei <br>
<br>
<br>
On 5/15/20 05:12, <a class="moz-txt-link-abbreviated"
href="mailto:coleen.phillimore@oracle.com"
moz-do-not-send="true">coleen.phillimore@oracle.com</a>
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:2f9aa92c-18f5-1203-1523-3c1fd9ba9ad1@oracle.com">
<br>
Serguei,<br>
<br>
Good find!! The fix looks good. I'm sure the
optimization wasn't noticeable and thank you for the
additional comments.<br>
<br>
There is a Method::external_name() function that I
believe prints all the things you want in the logging
here:<br>
<br>
<a
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/src/hotspot/share/oops/cpCache.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/src/hotspot/share/oops/cpCache.cpp.udiff.html</a><br>
<br>
I don't need to see another webrev if you make this
change.<br>
<br>
Thanks,<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 5/14/20 12:26 PM, <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:5942b42c-b9b3-f1d4-6c13-774649fca32b@oracle.com">
Please, review a fix for The Kitchensink bug:<br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8222005"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8222005</a><br>
<br>
Webrev:<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-redef.1/</a><br>
<br>
Summary:<br>
The VM_RedefineClasses::doit() uses two helper
classes to walk all VM classes.<br>
First is AdjustAndCleanMetadata to adjust method
entries in the vtables/itables/cpcaches.<br>
Second is CheckClass to check that adjustments for
all method entries are correct.<br>
The Kitchensink test is failing with two modes:<br>
- guarantee(false) failed: OLD and/or OBSOLETE
method(s) found in the<br>
VM_RedefineClasses::CheckClass::do_klass()<br>
- SIGSEGV in the
ConstantPoolCacheEntry::get_interesting_method_entry()
in context<br>
of VM_RedefineClasses::CheckClass::do_klass()
execution<br>
<br>
The second failure mode is rare. In is before the
first one in the code path.<br>
The root cause of both is that the
VM_RedefineClasses::AdjustAndCleanMetadata::do_klass()<br>
is skipping the cpcache update for classes that are
being redefined assuming they are<br>
being redefined by the current VM_RedefineClasses
operation. In such cases, the adjustment<br>
is not needed as the cpcache is empty. The problem
is that the assumption above is wrong.<br>
The class can also be redefined by another
VM_RedefineClasses operation <span class="changed">which
has already<br>
executed its doit_prologue</span>. The cpcache
djustment for such class is necessary.<br>
The fix is to always call the
cp_cache->adjust_method_entries() even if the class
is<br>
being redefined by the current VM_RedefineClasses
operation. It is possible to skip it<br>
but it will add extra complexity to the code.<br>
The fix also includes minor tweak in the cpCache.cpp
to include method's class name to<br>
the redefinition cpcache log.<br>
<br>
Testing:<br>
Ran Kitchensink test locally on a Linux server with
the Instrumentation module enabled.<br>
The test does not fail anymore.<br>
In progress, a mach5 tiers 1-5 and runs and separate
mach5 Kitchensink run.<br>
<br>
Thanks,<br>
Serguei<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>