[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:       Mikael Gerdin <mikael.gerdin () oracle ! com>
Date:       2013-09-30 12:51:15
Message-ID: 524973C3.4070003 () oracle ! com
[Download RAW message or body]

Fredrik,

On 09/30/2013 01:45 PM, 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);

It seems to me that the synchronization is only needed to make sure that 
we get a correct count for the _list array. New Klass*'s are pushed on 
the top of the _klasses list so that should not interfere with walking 
the Klass links.

One approach would be to do only one walk over the ClassLoaderDataGraph 
and use a growing datastructure instead of allocating one big linear 
array. I don't see any obvious requirement for a random-access 
datastructure so you could probably just put a Stack<Handle> there and 
push the mirrors on the stack, get the stack size and put them on 
result_list in the correct order.

You can also make JvmtiGetLoadedClassesClosure a KlassClosure and get 
rid of the thread-local get_this/set_this since ClassLoaderDataGraph 
provides an interface for a KlassClosure as well as a callback function 
pointer.

/Mikael

> 
> 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
> 


[prev in list] [next in list] [prev in thread] [next in thread]