Re: [RFA] JVMTI/JDWP GetAllLoadedClasses

[Keith Seitz <keiths at redhat dot com>]

A couple of comments...

Kyle Galloway wrote:
> Index: java/lang/natClassLoader.cc
> ===================================================================
> --- java/lang/natClassLoader.cc	(revision 124192)
> +++ java/lang/natClassLoader.cc	(working copy)
> @@ -380,6 +392,19 @@
>  void
>  _Jv_CopyClassesToSystemLoader (gnu::gcj::runtime::SystemClassLoader *loader)
>  {
> +  using namespace ::java::lang;
> +
> +  // Add any classes loaded before this point to the loaded classes list for
> +  // JVMTI.  As this is done, the memory used by the list is freed.
> +  for (struct _Jv_TempClassListElement *ptr = jvmti_class_list_head;
> +       ptr;
> +       ptr = ptr->next)
> +    {
> +      VMClassLoader::loaded_classes->add (new ref::WeakReference (ptr->klass));
> +      _Jv_Free (jvmti_class_list_head);
> +      jvmti_class_list_head = ptr;
> +    }
> +
>    for (jclass klass = system_class_list;
>         klass;
>         klass = klass->next_or_version)

In order to minimize the impact of this on startup times for the 
non-debug case, please wrap all this so that it does nothing when JVMTI 
is not active. You can check JVMTI::enabled to do this.

> @@ -655,6 +680,33 @@
>  _Jv_PushClass (jclass k)
>  {
>    JvSynchronize sync (&java::lang::Class::class$);
> +  
> +  using namespace ::java::lang;
> +
> +  // Build a list of loaded classes for JVMTI.
> +  if (system_class_list == SYSTEM_LOADER_INITIALIZED)
> +    VMClassLoader::loaded_classes->add (new ref::WeakReference (k));
> +  else
> +    {
> +      if (!jvmti_class_list_head)
> +        {
> +          jvmti_class_list_head 
> +            = reinterpret_cast<struct _Jv_TempClassListElement *> 
> +              (_Jv_MallocUnchecked (sizeof (struct _Jv_TempClassListElement)));
> +          jvmti_class_list_head->next = NULL;
> +        }
> +      else
> +        {
> +	  struct _Jv_TempClassListElement *tmp 
> +            = reinterpret_cast<struct _Jv_TempClassListElement *> 
> +              (_Jv_MallocUnchecked (sizeof (struct _Jv_TempClassListElement)));
> +          tmp->next = jvmti_class_list_head;
> +          jvmti_class_list_head = tmp;
> +        }
> +	
> +        jvmti_class_list_head->klass = k;
> +    }
> +  
>    jclass tmp = stack_head;
>    stack_head = k;
>    k->chain = tmp;

Likewise with JVMTI::enabled.

> Index: java/lang/VMClassLoader.h
> ===================================================================
> --- java/lang/VMClassLoader.h	(revision 124192)
> +++ java/lang/VMClassLoader.h	(working copy)

This header is regenerated by the build, right? I don't think you need 
to post a patch for it in that case. We all know it is regenerated. (And 
your ChangeLog told us it was regenerated. [Ditto the class files.] :-)

> Index: java/lang/VMClassLoader.java
> ===================================================================
> --- java/lang/VMClassLoader.java	(revision 124192)
> +++ java/lang/VMClassLoader.java	(working copy)
> @@ -345,4 +350,24 @@
>  
>      return default_sys;
>    }
> +
> +  /**
> +   * First check if any of the classes in the list have been collected, and if
> +   * they have, remove them from the list.  It then returns the modified list.
> +   *
> +   * @return The <code>HashSet</code> of loaded classes.
> +   */
> +  public static HashSet getAllLoadedClasses()
> +  {
> +    Iterator it = loaded_classes.iterator();
> +
> +    while (it.hasNext())
> +      {
> +        WeakReference ref = (WeakReference) it.next();
> +        if (ref.get() == null)
> +          it.remove();
> +      }
> +
> +    return loaded_classes;
> +  }
>  }

I have one concern about this... If another class is loaded by another 
thread at this time, your iterating functions 
(_Jv_JVMTI_GetLoadedClasses and getAllLoadedClasses) are going to throw 
a ConcurrentModificationException.

While this seems pretty unlikely today, sometime in the future it is 
conceivable that someone might run multiple JVMTI agents or have 
multiple threads running through JDWP (all suspended/running 
asynchronously). I would throw in a lock just to be sure. [I'm sure 
there are other parts around JDWP where this is an issue, but we have to 
start somewhere.]

Keith