<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<font size="+1">Dan, Thank you for reviewing the code. Most of the
files were easy.<br>
<br>
</font>
<div class="moz-cite-prefix">On 2/14/14 10:15 AM, Daniel D.
Daugherty wrote:<br>
</div>
<blockquote cite="mid:52FE3306.50900@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
On 2/13/14 12:00 PM, Coleen Phillimore wrote:<br>
<blockquote cite="mid:52FD1653.2030809@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<font size="+1">Summary: Remove search in system dictionary and
hacks, replace with verifying in CLD::_klasses list.<br>
<br>
open webrev at <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoleenp/8027146/">http://cr.openjdk.java.net/~coleenp/8027146/</a><br>
</font></blockquote>
<tt><font size="+1"> <br>
<font size="+1">src/share/vm/classfile/classLoaderData.<font
size="+1">h</font>pp<br>
<font size="+1"> No<font size="+1"> comments<font
size="+1"> other than why add a blank line on 273<font
size="+1">?<br>
</font></font></font></font></font></font></tt></blockquote>
<br>
<tt><font size="+1">No reason. I'll remove it. I might have made
and removed a different change there.<br>
<br>
</font></tt>
<blockquote cite="mid:52FE3306.50900@oracle.com" type="cite"><tt><font
size="+1"><font size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"> <br>
</font></font></font></font>src/share/vm/classfile/classLoaderData.<font
size="+1">c</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/classfile/dictionary.cpp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/classfile/systemDictionary.<font
size="+1">h</font>pp<br>
<font size="+1"> No com<font size="+1">ments.<br>
<br>
</font></font>src/share/vm/classfile/systemDictionary.<font
size="+1">c</font>pp<br>
<font size="+1"> So
SystemDictionary::verify_obj_klass_present() isn't even<br>
<font size="+1"> useful or correct in sanity
checking/debug code?<br>
</font></font></font></font></tt></blockquote>
<tt><font size="+1"><br>
I don't think it's that useful since there are so many
exceptions to this check and it has to lock the system dictionary
for the query. And it used to be called when verifying the
classes in the system dictionary.<br>
<br>
</font></tt>
<blockquote cite="mid:52FE3306.50900@oracle.com" type="cite"><tt><font
size="+1"><font size="+1"><font size="+1"><font size="+1"> <br>
<font size="+1"> Update: So
InstanceKlass::verify_on() now does a much simpler<br>
<font size="+1"> <font size="+1">contains_klass()
check. I <font size="+1">understand now why the
existing<br>
<font size="+1"> check needs to be
replaced by the simpler and less strict<br>
<font size="+1"> check.</font><br>
</font></font></font></font></font></font></font></font></font></tt></blockquote>
<br>
<tt><font size="+1">Yes, when we create the class we link it to it's
ClassLoaderData and vice versa. Making sure these links are
correct is worthwhile.<br>
<br>
</font></tt>
<blockquote cite="mid:52FE3306.50900@oracle.com" type="cite"><tt><font
size="+1"><font size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"> </font></font></font></font></font><br>
</font></font>src/share/vm/oops/arrayKlass.<font size="+1">h</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/arrayKlass.<font size="+1">c</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/instanceKlass.<font size="+1">h</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/instanceKlass.<font size="+1">c</font>pp<br>
<font size="+1"> The old c<font size="+1">ode didn't call
verify_obj_klass_present() <font size="+1">unless<br>
<font size="+1"> is_loaded() is tr<font size="+1">ue.
The new code presumes that <font size="+1">is_loaded()<br>
<font size="+1"> is true <font size="+1">since
it expects the class to be present in the<br>
<font size="+1"> class_loader_data.<br>
<br>
<font size="+1"> Update: <font
size="+1"><font size="+1">To put it
another way<font size="+1">: The old
code assume<font size="+1">d when<br>
<font size="+1"> is_loaded()
is true that the class was in
the system dictionary<br>
<font size="+1"> or in the
placeholder table. That's
not quite correct in all<br>
<font size="+1"> cases.
When is_loaded() i<font
size="+1">s true,<font
size="+1"> all we <font
size="+1">know for
sure <font
size="+1">is that<br>
<font size="+1">
the class must
be present in<font
size="+1"> the
class_loader_data();
it might<br>
<font
size="+1">
still be "on
its way" to
either the
system
dictionary or
the<br>
<font
size="+1">
placeholder
table, but has
not yet
arrived there.</font><br>
</font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></tt></blockquote>
<br>
<tt><font size="+1">Yes. </font></tt><tt><font size="+1">I think
this is the better verification.<br>
<br>
Thanks!<br>
Coleen<br>
</font></tt>
<blockquote cite="mid:52FE3306.50900@oracle.com" type="cite"><tt><font
size="+1"><font size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"><font size="+1"><font
size="+1"><font size="+1"><font
size="+1"><font size="+1"><font
size="+1"><font size="+1"><font
size="+1"><font
size="+1"><font
size="+1"><font
size="+1"><font
size="+1"><font
size="+1"><font
size="+1"> </font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font></font><br>
</font>src/share/vm/oops/klass.<font size="+1">h</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/klass.<font size="+1">c</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/objArrayKlass.<font size="+1">h</font>pp<br>
<font size="+1"> No comments.<br>
<br>
</font>src/share/vm/oops/objArrayKlass.<font size="+1">c</font>pp<br>
No comments.<br>
<br>
Thumbs up.<br>
<br>
<font size="+1">Dan<br>
<br>
<br>
</font><font size="+1"> </font><br>
</font></font></tt>
<blockquote cite="mid:52FD1653.2030809@oracle.com" type="cite"><font
size="+1"> bug link <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8027146">https://bugs.openjdk.java.net/browse/JDK-8027146</a><br>
<br>
Tested with AllocClasses.java with VM_Verify op suggested in
the bug report (can't add it's noreg-hard). Also ran all
jtreg and gc jtreg tests with -XX:+VerifyBeforeGC.<br>
<br>
Thanks,<br>
Coleen<br>
</font> </blockquote>
<br>
</blockquote>
<br>
</body>
</html>