[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR (S): JDK-8024423: JVMTI: GetLoadedClasses doesn't enumerate anonymous classes
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2013-10-23 8:18:14
Message-ID: 52678646.9050805 () oracle ! com
[Download RAW message or body]
Fredrik,
The fix is good, good job!
It is still a good idea to run the nsk.jvmti and nsk.jdi tests to be safe.
Thanks,
Serguei
On 10/23/13 1:04 AM, Fredrik Arvidsson wrote:
> Hi
>
> The latest and hopefully last review version of this change can be
> found here:
> http://cr.openjdk.java.net/~farvidsson/8024423/webrev.03/
> <http://cr.openjdk.java.net/%7Efarvidsson/8024423/webrev.03/>
>
> Cheers
> /F
>
> On 2013-10-22 17:11, Stefan Karlsson wrote:
> > On 10/17/13 4:02 PM, Fredrik Arvidsson wrote:
> > > Hi
> > >
> > > I have added a new revision of my changes here:
> > > http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/
> > > <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.02/>
> >
> > I haven't looked at the patch i great detail, but I found this code:
> >
> > + void LoadedClassesClosure::do_klass(Klass* k) {
> > + // Collect all jclasses
> > + for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) {
> > + oop mirror = l->java_mirror();
> > + _classStack.push((jclass) _env->jni_reference(mirror));
> > + }
> > + }
> >
> > I think you are adding the array klasses multiple times.
> > ClassLoaderData::loaded_classes_do() is visiting all array klasses,
> > so you shouldn't be traversing the l->array_klass_or_null() chain.
> >
> > StefanK
> >
> > > The idea is to use a lock called metaspace_lock available inside the
> > > ClassLoaderData objects that the ClassLoaderDataGraph holds, this
> > > prevents classes to be added/removed/updated during the gathering
> > > phase. When iterating using the new LoadedClassesClosure
> > > implementation only array classes and instance classes that are
> > > marked as loaded will be visited. The LoadedClassesClosure
> > > implementation uses a Stack<jclass> to store classes during the
> > > gathering phase. This changes the count-allocate-add-extract
> > > approach that was used before into a add-extract way of doing it
> > > instead.
> > >
> > > I am still not sure how to do with the GetClassLoaderClasses to make
> > > it consistent. I would eventually like to get rid of the
> > > JvmtiGetLoadedClassesClosure and the use of the SystemDictionary if
> > > possible. But right now I just cant see how to get hold of the
> > > initiating loader for a class without using the SystemDictionary.
> > >
> > > /Fredrik
> > >
> > > On 2013-10-15 09:42, serguei.spitsyn@oracle.com wrote:
> > > >
> > > > There are two questions on the list that we still need to resolve
> > > > in this fix:
> > > > (1) What is the best way to protect the work with CLDG from
> > > > concurrent additions of CLD's to it
> > > > (2) The *GetClassLoaderClasses* needs a fix as well to be
> > > > consistent with the GetLoadedClasses
> > > >
> > > > I had some private conversations with Fredrik and John Rose and
> > > > after some analysis came up with the suggestion:
> > > >
> > > > (1) Continue using the *SystemDictionary_lock* to protect
> > > > consistency of the loaded classes data.
> > > > The issue is that adding CLD's to the SLDG is not currently
> > > > protected by the *SystemDictionary_lock*.
> > > > I'm suggesting to add it to the
> > > > *SystemDictionary::parse_stream* (please, see the webrev below).
> > > >
> > > > (2) There was some doubt that a similar fix for the
> > > > *GetClassLoaderClasses* is needed because
> > > > the CL+SD (taken from the host class) does not reference the
> > > > associated anonymous classes.
> > > > The question is: Can we consider the host's class CL as the
> > > > initiating CL?
> > > > My reading is that the answer to this question is: "Yes, we
> > > > can".
> > > > Even though the CL itself does not have references to its
> > > > anonymous classes,
> > > > there are references from the anonymous classes's CLD's to
> > > > its CL.
> > > > These references can be used in the *increment_with_loader*
> > > > and *add_with_loader*
> > > > the same way as it was before.
> > > >
> > > > This is a webrev that includes the Fredrik's fix as a base plus the
> > > > implemented suggestion:
> > > > http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/
> > > >
> > > > Some help from the HotSpot team is needed to estimate if this
> > > > approach is Ok and does not have rat holes in it.
> > > > Opinions are very welcome.
> > > >
> > > > Thanks,
> > > > Serguei
> > > >
> > > >
> > > > On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:
> > > > > Hi and thanks for all your comments.
> > > > >
> > > > > I have simplified the code in *instanceKlass.cpp* like suggested
> > > > > and here is a new webrev:
> > > > > http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/
> > > > > <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.01/>
> > > > >
> > > > > Regarding the need to synchronize the access to
> > > > > ClassLoaderDataGraph I have come to the conclusion that it should
> > > > > be safe to call this without any additional synchronization since
> > > > > newly loaded and added classes are appended to the end of its
> > > > > 'list' of classes. This would mean that the call could 'miss' the
> > > > > classes added during the collection phase, but this is not a big
> > > > > issue. I hope that my conclusion is correct.
> > > > >
> > > > > I believe that the
> > > > > JvmtiGetLoadedClasses::getClassLoaderClasses(...) method is to be
> > > > > left alone this time since I got the impression that only
> > > > > SystemDictionary 'knows' about initiating class loaders.
> > > > >
> > > > > There is plenty of room for improvement and simplification of much
> > > > > of the code in this area, but I feel that it must wait until
> > > > > another time. The task to re-factor and simplify would quickly
> > > > > grow out of hands I'm afraid :)
> > > > >
> > > > > Cheers
> > > > > /Fredrik
> > > > >
> > > > > On 2013-10-01 09:34, serguei.spitsyn@oracle.com wrote:
> > > > > > Hi Frederik,
> > > > > >
> > > > > >
> > > > > > Thank you for jumping on this issue!
> > > > > >
> > > > > >
> > > > > > *src/share/vm/oops/instanceKlass.cpp*
> > > > > > 2333 int src_index = 0;
> > > > > > 2334 while (src_index < src_length) {
> > > > > > 2335 dest[dest_index++] = src[src_index++];
> > > > > > 2336 }
> > > > > > 2337
> > > > > > 2338 // If we have a hash, append it
> > > > > > 2339 if(hash_len > 0) {
> > > > > > 2340 int hash_index = 0;
> > > > > > 2341 while (hash_index < hash_len) {
> > > > > > 2342 dest[dest_index++] = hash_buf[hash_index++];
> > > > > > 2343 }
> > > > > > 2344 }
> > > > > >
> > > > > > The above can be simplified a little bit:
> > > > > > // Add the actual class name
> > > > > > for (int src_index = 0; src_index < src_length; ) {
> > > > > > dest[dest_index++] = src[src_index++];
> > > > > > }
> > > > > > // If we have a hash, append it
> > > > > > for (int hash_index = 0; hash_index < hash_len; ) {
> > > > > > dest[dest_index++] = hash_buf[hash_index++];
> > > > > > }
> > > > > >
> > > > > > The conditionif(hash_len > 0) is covered by the loop itself.
> > > > > >
> > > > > > Style-related comments:
> > > > > > -wrong indent at the lines: 2316-2319
> > > > > > - need a space after the 'if' at the lines: 2315, 2339
> > > > > >
> > > > > >
> > > > > > *src/share/vm/prims/jvmtiGetLoadedClasses.cpp
> > > > > >
> > > > > > *The copyright comment must be up-to-date.
> > > > > > The comments at the lines 35-38, 258-260 are not valid anymore.
> > > > > >
> > > > > >
> > > > > > > In this review I still have an outstanding unanswered question about
> > > > > > > the need to synchronize the access to the:
> > > > > >
> > > > > > > ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
> > > > > > > and
> > > > > > > ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);
> > > > > >
> > > > > > This kind of walking is usually done at safepoint in a VM_Operation.
> > > > > > You will find many examples in the jvmtiEnv.cpp, for instance, \
> > > > > > VM_GetAllStackTraces. The suggestion from Mikael is good too.
> > > > > >
> > > > > > Should this fix include the getClassLoaderClasses() as well?
> > > > > > I'm still trying to realize the impact and consistency of this fix. :(
> > > > > >
> > > > > > What tests are you going to run for verification?
> > > > > >
> > > > > > Thanks,
> > > > > > Serguei
> > > > > > *
> > > > > > *On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:
> > > > > > > Hi
> > > > > > >
> > > > > > > Please help me to review the changes below:
> > > > > > >
> > > > > > > Jira case: https://bugs.openjdk.java.net/browse/JDK-8024423
> > > > > > > Webrev:
> > > > > > > http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/
> > > > > > > <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/>
> > > > > > >
> > > > > > > In this review I still have an outstanding unanswered question
> > > > > > > about the need to synchronize the access to the:
> > > > > > > ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
> > > > > > > and
> > > > > > > ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);
> > > > > > >
> > > > > > > calls in
> > > > > > > http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html \
> > > > > > > <http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html>
> > > > > > >
> > > > > > > Please give me advise on this.
> > > > > > >
> > > > > > > There will be a review soon regarding re-enabling the tests that failed \
> > > > > > > because of the above bug, see: \
> > > > > > > https://bugs.openjdk.java.net/browse/JDK-8025576
> > > > > > > Cheers
> > > > > > > /Fredrik
> > > > > >
> > > > >
> > > >
> > >
> >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Fredrik,<br>
<br>
The fix is good, good job!<br>
It is still a good idea to run the nsk.jvmti and nsk.jdi tests to
be safe.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 10/23/13 1:04 AM, Fredrik Arvidsson wrote:<br>
</div>
<blockquote cite="mid:5267831D.4060101@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi<br>
<br>
The latest and hopefully last review version of this change can be
found here: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Efarvidsson/8024423/webrev.03/">http://cr.openjdk.java.net/~farvidsson/8024423/webrev.03/</a><br>
<br>
Cheers<br>
/F<br>
<br>
<div class="moz-cite-prefix">On 2013-10-22 17:11, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:526695B0.2000402@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 10/17/13 4:02 PM, Fredrik
Arvidsson wrote:<br>
</div>
<blockquote cite="mid:525FEE10.9030508@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi<br>
<br>
I have added a new revision of my changes here: <a
moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.02/">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.02/</a><br>
</blockquote>
<br>
I haven't looked at the patch i great detail, but I found this
code:<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre><span class="new">+ void LoadedClassesClosure::do_klass(Klass* k) \
{</span> <span class="new">+ // Collect all jclasses</span>
<span class="new">+ for (Klass* l = k; l != NULL; l = l->array_klass_or_null()) \
{</span> <span class="new">+ oop mirror = l->java_mirror();</span>
<span class="new">+ _classStack.push((jclass) \
_env->jni_reference(mirror));</span> <span class="new">+ }</span>
<span class="new">+ }
</span></pre>
I think you are adding the array klasses multiple times.
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
ClassLoaderData::loaded_classes_do() is visiting all array
klasses, so you shouldn't be traversing the
l->array_klass_or_null() chain.<br>
<br>
StefanK<br>
<br>
<blockquote cite="mid:525FEE10.9030508@oracle.com" type="cite">
The idea is to use a lock called metaspace_lock available
inside the ClassLoaderData objects that the
ClassLoaderDataGraph holds, this prevents classes to be
added/removed/updated during the gathering phase. When
iterating using the new LoadedClassesClosure implementation
only array classes and instance classes that are marked as
loaded will be visited. The LoadedClassesClosure
implementation uses a Stack<jclass> to store classes
during the gathering phase. This changes the
count-allocate-add-extract approach that was used before into
a add-extract way of doing it instead.<br>
<br>
I am still not sure how to do with the GetClassLoaderClasses
to make it consistent. I would eventually like to get rid of
the JvmtiGetLoadedClassesClosure and the use of the
SystemDictionary if possible. But right now I just cant see
how to get hold of the initiating loader for a class without
using the SystemDictionary.<br>
<br>
/Fredrik<br>
<br>
<div class="moz-cite-prefix">On 2013-10-15 09:42, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:525CF1F1.1010400@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
There are two questions on the list that we still need to
resolve in this fix:<br>
(1) What is the best way to protect the work with CLDG
from concurrent additions of CLD's to it<br>
(2) The <b>GetClassLoaderClasses</b> needs a fix as well
to be consistent with the GetLoadedClasses<br>
<br>
I had some private conversations with Fredrik and John
Rose and<br>
after some analysis came up with the suggestion:<br>
<br>
(1) Continue using the <b>SystemDictionary_lock</b> to
protect consistency of the loaded classes data.<br>
The issue is that adding CLD's to \
the SLDG is not currently protected by the <b>SystemDictionary_lock</b>.<br>
I'm suggesting to add it to the \
<b>SystemDictionary::parse_stream</b> (please, see the webrev below).<br>
<br>
(2) There was some doubt that a similar fix for the \
<b>GetClassLoaderClasses</b> is needed because<br>
the CL+SD (taken from the host \
class) does not reference the associated anonymous classes.<br>
The question is: Can we consider \
the host's class CL as the initiating CL?<br>
My reading is that the answer to \
this question is: "Yes, we can".<br>
Even though the CL itself does not \
have references to its anonymous classes,<br>
there are references from the anonymous \
classes's CLD's to its CL.<br>
These references can be used in \
the <b>increment_with_loader</b> and <b>add_with_loader</b><br>
the same way as it was before.<br>
<br>
This is a webrev that includes the Fredrik's fix as a base
plus the implemented suggestion:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/80244 \
23-JVMTI-ANO/">http://javaweb.sfbay.sun.com/java/svc/ss45998/webrevs/2013/hotspot/fredrik/8024423-JVMTI-ANO/</a><br>
<br>
Some help from the HotSpot team is needed to estimate if
this approach is Ok and does not have rat holes in it.<br>
Opinions are very welcome.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/2/13 2:28 AM, Fredrik Arvidsson wrote:<br>
</div>
<blockquote cite="mid:524BE733.2070205@oracle.com"
type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
Hi and thanks for all your comments.<br>
<br>
I have simplified the code in <b>instanceKlass.cpp</b>
like suggested and here is a new webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.01/">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.01/</a><br>
<br>
Regarding the need to synchronize the access to
ClassLoaderDataGraph I have come to the conclusion that it
should be safe to call this without any additional
synchronization since newly loaded and added classes are
appended to the end of its 'list' of classes. This would
mean that the call could 'miss' the classes added during
the collection phase, but this is not a big issue. I hope
that my conclusion is correct.<br>
<br>
I believe that the
JvmtiGetLoadedClasses::getClassLoaderClasses(...) method
is to be left alone this time since I got the impression
that only SystemDictionary 'knows' about initiating class
loaders.<br>
<br>
There is plenty of room for improvement and simplification
of much of the code in this area, but I feel that it must
wait until another time. The task to re-factor and
simplify would quickly grow out of hands I'm afraid :)<br>
<br>
Cheers<br>
/Fredrik<br>
<br>
<div class="moz-cite-prefix">On 2013-10-01 09:34, <a
moz-do-not-send="true"
class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:524A7B07.1000300@oracle.com"
type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Frederik,<br>
<br>
<br>
Thank you for jumping on this issue!<br>
<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<b>src/share/vm/oops/instanceKlass.cpp</b><br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<pre><span class="new">2333 int src_index = 0;</span>
2334 while (src_index < src_length) {
2335 dest[dest_index++] = src[src_index++];
2336 }
<span class="new">2337 </span>
<span class="new">2338 // If we have a hash, append it</span>
<span class="new">2339 if(hash_len > 0) {</span>
<span class="new">2340 int hash_index = 0;</span>
<span class="new">2341 while (hash_index < hash_len) {</span>
<span class="new">2342 dest[dest_index++] = hash_buf[hash_index++];</span>
<span class="new">2343 }</span>
<span class="new">2344 }
The above can be simplified a little bit:
// Add the actual class name
for (int src_index = 0; src_index < src_length; ) {
dest[dest_index++] = src[src_index++];
}
// If we have a hash, append it
for (int hash_index = 0; hash_index < hash_len; ) {
dest[dest_index++] = hash_buf[hash_index++];
}
The condition </span><span class="new">if(hash_len > 0)</span> is covered by the \
loop itself.
Style-related comments:
- <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"><span \
class="new">wrong indent at the lines: 2316</span>-2319
- need a space after the 'if' at the lines: 2315, 2339
<meta http-equiv="content-type" content="text/html; \
charset=ISO-8859-1"><b>src/share/vm/prims/jvmtiGetLoadedClasses.cpp
</b>The copyright comment must be up-to-date.
The comments at the lines 35-38, 258-260 are not valid anymore.
> In this review I still have an outstanding unanswered question about
> the need to synchronize the access to the:
<span class="new" style="color: blue; font-weight: normal;">> \
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment); <font \
color="#000000">> and</font> </span><span class="new" style="color: blue; \
font-weight: normal;">> \
ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);</span>
This kind of walking is usually done at safepoint in a VM_Operation.
You will find many examples in the jvmtiEnv.cpp, for instance, VM_GetAllStackTraces.
The suggestion from Mikael is good too.
Should this fix include the getClassLoaderClasses() as well?
I'm still trying to realize the impact and consistency of this fix. :(
What tests are you going to run for verification?
Thanks,
Serguei
<b>
</b>On 9/30/13 4:45 AM, Fredrik Arvidsson wrote:
</pre>
</div>
<blockquote cite="mid:5249643F.3030209@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
Hi<br>
<br>
Please help me to review the changes below:<br>
<br>
Jira case: <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8024423">https://bugs.openjdk.java.net/browse/JDK-8024423</a><br>
Webrev: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/</a><br>
<br>
In this review I still have an outstanding unanswered
question about the need to synchronize the access to
the:<br>
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: \
normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; \
word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span class="new" style="color: \
blue; font-weight: normal;">ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::increment);
<font color="#000000">and</font>
</span><span class="new" style="color: blue; font-weight: \
normal;">ClassLoaderDataGraph::classes_do(&JvmtiGetLoadedClassesClosure::add);</span>
calls in
<a moz-do-not-send="true" \
href="http://cr.openjdk.java.net/%7Eallwin/farvidss/8024423/webrev.00/src/share/vm/pri \
ms/jvmtiGetLoadedClasses.cpp.udiff.html">http://cr.openjdk.java.net/~allwin/farvidss/8024423/webrev.00/src/share/vm/prims/jvmtiGetLoadedClasses.cpp.udiff.html</a>
Please give me advise on this.
There will be a review soon regarding re-enabling the tests that failed because of \
the above bug, see: <a moz-do-not-send="true" \
href="https://bugs.openjdk.java.net/browse/JDK-8025576">https://bugs.openjdk.java.net/browse/JDK-8025576</a>
Cheers
/Fredrik
</pre>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</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