[RFC] JVMTI CLASS_PREPARE event

[RFC] JVMTI CLASS_PREPARE event

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

Hi,

If anyone was wondering when the patches were going require serious 
review, your wait is over. I've not generated a "proper" diff using "svn 
diff" for this "patch" because it is way too verbose with whitespace 
changes. Instead, I've simply used "diff -uw" on it. It's much more 
readable for purposes of this discussion.

The patch adds the JVMTI event notification for CLASS_PREPARE. This is 
now the third or fourth place I've put this notification (at least), and 
I know this is still not quite right. It's awfully close, but not perfect.

I welcome suggestions, advice, comments, etc.

Keith

[-- Attachment #2: classprepare-event.patch --]
[-- Type: text/x-patch, Size: 954 bytes --]

--- .svn/text-base/link.cc.svn-base	2007-01-10 18:35:41.000000000 -0800
+++ link.cc	2007-01-24 13:09:11.000000000 -0800
@@ -33,6 +33,8 @@
 #include <limits.h>
 #include <java-cpool.h>
 #include <execution.h>
+#include <jvmti.h>
+#include "jvmti-int.h"
 #include <java/lang/Class.h>
 #include <java/lang/String.h>
 #include <java/lang/StringBuffer.h>
@@ -1941,6 +1943,7 @@
   if (klass->state >= state)
     return;
 
+  {
   JvSynchronize sync (klass);
 
   // This is similar to the strategy for class initialization.  If we
@@ -2027,3 +2030,15 @@
   if (klass->state == JV_STATE_ERROR)
     throw new java::lang::LinkageError;
 }
+
+  if (klass->state == JV_STATE_LINKED)
+    {
+      if (JVMTI_REQUESTED_EVENT (ClassPrepare))
+	{
+	  java::lang::Thread *thread = java::lang::Thread::currentThread ();
+	  JNIEnv *jni_env = _Jv_GetCurrentJNIEnv ();
+	  _Jv_JVMTI_PostEvent (JVMTI_EVENT_CLASS_PREPARE, thread, jni_env,
+			       klass);
+	}
+    }
+}

Re: [RFC] JVMTI CLASS_PREPARE event

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> The patch adds the JVMTI event notification for CLASS_PREPARE. This is
Keith> now the third or fourth place I've put this notification (at least),
Keith> and I know this is still not quite right. It's awfully close, but not
Keith> perfect.

Keith> I welcome suggestions, advice, comments, etc.

This looks good to me.

What is it you need to capture here?  Maybe we can suggest a better
spot.  Though, almost all state transitions happen here, so this is
pretty good.

Maybe wrap the condition in a builtin_expect?

Keith> +	  java::lang::Thread *thread = java::lang::Thread::currentThread ();

You could hoist the local variable 'self' outside the new scope and
reuse it.  If you're into micro-optimization and all.

Tom

Re: [RFC] JVMTI CLASS_PREPARE event

[Keith Seitz]

Tom Tromey wrote:

> Keith> I welcome suggestions, advice, comments, etc.
> 
> This looks good to me.
> 
> What is it you need to capture here?  Maybe we can suggest a better
> spot.  Though, almost all state transitions happen here, so this is
> pretty good.

We need to be able to report when the VM has prepared a class. My 
biggest concern about this patch is if any class runs through 
_Jv_Linker::wait_for_state without ever "stopping" at JV_STATE_LINKED, 
i.e., it goes to JV_STATE_DONE. I don't know if this is even possible, 
though. If it can't happen, then this should work just fine.

> Maybe wrap the condition in a builtin_expect?

I'll do that.

> Keith> +	  java::lang::Thread *thread = java::lang::Thread::currentThread ();
> 
> You could hoist the local variable 'self' outside the new scope and
> reuse it.  If you're into micro-optimization and all.

Done.

Keith

Re: [RFC] JVMTI CLASS_PREPARE event

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> We need to be able to report when the VM has prepared a class. My
Keith> biggest concern about this patch is if any class runs through
Keith> _Jv_Linker::wait_for_state without ever "stopping" at JV_STATE_LINKED,
Keith> i.e., it goes to JV_STATE_DONE. I don't know if this is even possible,
Keith> though. If it can't happen, then this should work just fine.

I think it isn't really possible.  wait_for_state is not used for the
final transition to DONE; that is only done by the class
initialization code (plus the predictable special case for array
classes, which are born DONE).  However, all earlier transitions are
handled via wait_for_state.

If you want extra checking I think adding an '&& state >= JV_STATE_LINKED'
would do it.  We know that the original klass->state is '< state'
at the point where your code is added, due to the check at the top of
the function.

Tom

Re: [RFC] JVMTI CLASS_PREPARE event

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

Tom Tromey wrote:

> If you want extra checking I think adding an '&& state >= JV_STATE_LINKED'
> would do it.  We know that the original klass->state is '< state'
> at the point where your code is added, due to the check at the top of
> the function.

Doh! I don't know why I didn't even see that. The old "can't see the 
forest for the trees" thing, I guess.

Okay, so time to turn this into a proper patch: I've attached two files, 
one of the "svn diff" output that can be applied to the repository, the 
other a similar "diff -uw" of the revision (it's just *so* much easier 
to read).

Keith

ChangeLog
2007-01-24  Keith Seitz  <keiths@redhat.com>

         * link.cc (_Jv_Linker::wait_for_state): Add JVMTI
         CLASS_PREPARE notification.


[-- Attachment #2: classprepare-event-easy.patch --]
[-- Type: text/x-patch, Size: 1195 bytes --]

--- .svn/text-base/link.cc.svn-base	2007-01-10 18:35:41.000000000 -0800
+++ link.cc	2007-01-24 15:34:49.000000000 -0800
@@ -33,6 +33,8 @@
 #include <limits.h>
 #include <java-cpool.h>
 #include <execution.h>
+#include <jvmti.h>
+#include "jvmti-int.h"
 #include <java/lang/Class.h>
 #include <java/lang/String.h>
 #include <java/lang/StringBuffer.h>
@@ -1941,11 +1943,13 @@
   if (klass->state >= state)
     return;
 
+  java::lang::Thread *self = java::lang::Thread::currentThread();
+
+  {
   JvSynchronize sync (klass);
 
   // This is similar to the strategy for class initialization.  If we
   // already hold the lock, just leave.
-  java::lang::Thread *self = java::lang::Thread::currentThread();
   while (klass->state <= state
 	 && klass->thread 
 	 && klass->thread != self)
@@ -2027,3 +2031,13 @@
   if (klass->state == JV_STATE_ERROR)
     throw new java::lang::LinkageError;
 }
+
+  if (__builtin_expect (klass->state == JV_STATE_LINKED, false)
+      && state >= JV_STATE_LINKED
+      && JVMTI_REQUESTED_EVENT (ClassPrepare))
+    {
+      JNIEnv *jni_env = _Jv_GetCurrentJNIEnv ();
+      _Jv_JVMTI_PostEvent (JVMTI_EVENT_CLASS_PREPARE, self, jni_env,
+			   klass);
+    }
+}

[-- Attachment #3: classprepare-event-svn.patch --]
[-- Type: text/x-patch, Size: 4794 bytes --]

Index: link.cc
===================================================================
--- link.cc	(revision 121081)
+++ link.cc	(working copy)
@@ -33,6 +33,8 @@
 #include <limits.h>
 #include <java-cpool.h>
 #include <execution.h>
+#include <jvmti.h>
+#include "jvmti-int.h"
 #include <java/lang/Class.h>
 #include <java/lang/String.h>
 #include <java/lang/StringBuffer.h>
@@ -1941,33 +1943,35 @@
   if (klass->state >= state)
     return;
 
-  JvSynchronize sync (klass);
-
-  // This is similar to the strategy for class initialization.  If we
-  // already hold the lock, just leave.
   java::lang::Thread *self = java::lang::Thread::currentThread();
-  while (klass->state <= state
-	 && klass->thread 
-	 && klass->thread != self)
-    klass->wait ();
 
-  java::lang::Thread *save = klass->thread;
-  klass->thread = self;
+  {
+    JvSynchronize sync (klass);
 
-  // Allocate memory for static fields and constants.
-  if (GC_base (klass) && klass->fields && ! GC_base (klass->fields))
-    {
-      jsize count = klass->field_count;
-      if (count)
-	{
-	  _Jv_Field* fields 
-	    = (_Jv_Field*) _Jv_AllocRawObj (count * sizeof (_Jv_Field));
-	  memcpy ((void*)fields,
-		  (void*)klass->fields,
-		  count * sizeof (_Jv_Field));
-	  klass->fields = fields;
-	}
-    }
+    // This is similar to the strategy for class initialization.  If we
+    // already hold the lock, just leave.
+    while (klass->state <= state
+	   && klass->thread 
+	   && klass->thread != self)
+      klass->wait ();
+
+    java::lang::Thread *save = klass->thread;
+    klass->thread = self;
+
+    // Allocate memory for static fields and constants.
+    if (GC_base (klass) && klass->fields && ! GC_base (klass->fields))
+      {
+	jsize count = klass->field_count;
+	if (count)
+	  {
+	    _Jv_Field* fields 
+	      = (_Jv_Field*) _Jv_AllocRawObj (count * sizeof (_Jv_Field));
+	    memcpy ((void*)fields,
+		    (void*)klass->fields,
+		    count * sizeof (_Jv_Field));
+	    klass->fields = fields;
+	  }
+      }
       
   // Print some debugging info if requested.  Interpreted classes are
   // handled in defineclass, so we only need to handle the two
@@ -1981,49 +1985,59 @@
       ++gcj::loadedClasses;
     }
 
-  try
-    {
-      if (state >= JV_STATE_LOADING && klass->state < JV_STATE_LOADING)
-	{
-	  ensure_supers_installed (klass);
-	  klass->set_state(JV_STATE_LOADING);
-	}
+    try
+      {
+	if (state >= JV_STATE_LOADING && klass->state < JV_STATE_LOADING)
+	  {
+	    ensure_supers_installed (klass);
+	    klass->set_state(JV_STATE_LOADING);
+	  }
 
-      if (state >= JV_STATE_LOADED && klass->state < JV_STATE_LOADED)
-	{
-	  ensure_method_table_complete (klass);
-	  klass->set_state(JV_STATE_LOADED);
-	}
+	if (state >= JV_STATE_LOADED && klass->state < JV_STATE_LOADED)
+	  {
+	    ensure_method_table_complete (klass);
+	    klass->set_state(JV_STATE_LOADED);
+	  }
 
-      if (state >= JV_STATE_PREPARED && klass->state < JV_STATE_PREPARED)
-	{
-	  ensure_fields_laid_out (klass);
-	  make_vtable (klass);
-	  layout_interface_methods (klass);
-	  prepare_constant_time_tables (klass);
-	  klass->set_state(JV_STATE_PREPARED);
-	}
+	if (state >= JV_STATE_PREPARED && klass->state < JV_STATE_PREPARED)
+	  {
+	    ensure_fields_laid_out (klass);
+	    make_vtable (klass);
+	    layout_interface_methods (klass);
+	    prepare_constant_time_tables (klass);
+	    klass->set_state(JV_STATE_PREPARED);
+	  }
 
-      if (state >= JV_STATE_LINKED && klass->state < JV_STATE_LINKED)
-	{
-	  if (gcj::verifyClasses)
-	    verify_class (klass);
+	if (state >= JV_STATE_LINKED && klass->state < JV_STATE_LINKED)
+	  {
+	    if (gcj::verifyClasses)
+	      verify_class (klass);
 
-	  ensure_class_linked (klass);
-	  link_exception_table (klass);
-	  link_symbol_table (klass);
-	  klass->set_state(JV_STATE_LINKED);
-	}
-    }
-  catch (java::lang::Throwable *exc)
+	    ensure_class_linked (klass);
+	    link_exception_table (klass);
+	    link_symbol_table (klass);
+	    klass->set_state(JV_STATE_LINKED);
+	  }
+      }
+    catch (java::lang::Throwable *exc)
+      {
+	klass->thread = save;
+	klass->set_state(JV_STATE_ERROR);
+	throw exc;
+      }
+
+    klass->thread = save;
+
+    if (klass->state == JV_STATE_ERROR)
+      throw new java::lang::LinkageError;
+  }
+
+  if (__builtin_expect (klass->state == JV_STATE_LINKED, false)
+      && state >= JV_STATE_LINKED
+      && JVMTI_REQUESTED_EVENT (ClassPrepare))
     {
-      klass->thread = save;
-      klass->set_state(JV_STATE_ERROR);
-      throw exc;
+      JNIEnv *jni_env = _Jv_GetCurrentJNIEnv ();
+      _Jv_JVMTI_PostEvent (JVMTI_EVENT_CLASS_PREPARE, self, jni_env,
+			   klass);
     }
-
-  klass->thread = save;
-
-  if (klass->state == JV_STATE_ERROR)
-    throw new java::lang::LinkageError;
 }

Re: [RFC] JVMTI CLASS_PREPARE event

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Okay, so time to turn this into a proper patch: I've attached two
Keith> files, one of the "svn diff" output that can be applied to the
Keith> repository, the other a similar "diff -uw" of the revision (it's just
Keith> *so* much easier to read).

Ok, thanks.

You're right, the -uw version is way more readable.  Thanks for doing
that.

Tom

Re: [RFC] JVMTI CLASS_PREPARE event

[Keith Seitz]

Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> Okay, so time to turn this into a proper patch: I've attached two
> Keith> files, one of the "svn diff" output that can be applied to the
> Keith> repository, the other a similar "diff -uw" of the revision (it's just
> Keith> *so* much easier to read).
> 
> Ok, thanks.

Committed.

> You're right, the -uw version is way more readable.  Thanks for doing
> that.

No problem. It's tough enough (or at least time consuming enough) to 
review patches, much less to have the real meat of it hidden in a bunch 
of whitespace changes.

Keith