Re: portable-native-sync-5.patch

[Mark Wielaard]

Hi Steven,

On Fri, 2004-05-21 at 21:25, Steven Augart wrote:
> I first submitted this patch eleven days ago.  I've gotten positive 
> feedback and only minor suggestions on previous versions of it.

Sorry for taking so long to review this.
But it is huge patch (at least for my poor untrained C eyes).

Although I should have reviewed this earlier, one polite request to try
to split something like this up in multiple parts if possible.
There are some generally useful utility functions in
gnu_java_awt_peer_gtk_GtkMainThread.c that could have been split out and
put into libclasspath (native/jni/classpath) since I think they are
usable for other native code. (Sadly we are not very good at reuse in
the JNI code. And the Hacker Guide doesn't have much on libclasspath
(there are a few short words under 8. Programming Goals.)

So lets start with that part. You don't have to rip it out and/or merge
it with what is in libclasspath already since that would be much work
for something that now basically works. But if you could add a little
note to Chapter 8 about "more utility classes being available that could
be factored out if someone wanted something nice to hack on" that would
be appreciated. The error reporting and exception throwing functions
look nice enough to not want to reuse. Thanks.

Some small comments on that section:

> + *  - This file is only needed for --portable-native-sync.  We
> + *    conditionally compile it against the PORTABLE_NATIVE_SYNC macro.  
> Should
> + *    we manipulate Makefile.am instead?  Or should we go ahead and compile
> + *    it so that something which breaks this code will not be used?

In the future we should do the second. And provide some some VM hook to
either get this or the standard gtk_threads funtionality.

> (-Wconversion is not enabled by default for GNU Classpath,
> + *    but it is in my own CFLAGS, which, for gcc 3.3.3, read: -pipe -ggdb3 -W
> + *    -Wall -Wbad-function-cast -Wcast-align -Wpointer-arith -Wcast-qual
> + *    -Wshadow -Wstrict-prototypes -Wmissing-prototypes 
> -Wmissing-declarations
> + *    -fkeep-static-consts -fkeep-inline-functions -Wundef -Wwrite-strings
> + *    -Wno-aggregate-return -Wmissing-noreturn -Wnested-externs -Wtrigraphs
> + *    -Wconversion -Wsign-compare -Wno-float-equal -Wmissing-format-attribute
> + *    -Wno-unreachable-code -Wdisabled-optimization )

Please feel free to add any of these to the standard flags set in
configure.ac if they are not yet enabled. We are not completely warning
free though. So at the moment they might get lost between some other
warnings.

> +#define DELETE_LOCAL_REFS   0        /* Whether to delete local references.  
>  
> +
> +                                JNI only guarantees that there wil be 16
> +                                available.  (Jikes RVM provides an number
> +                                only limited by VM memory.)
> +
> +                                Jikes RVM will probably perform faster if
> +                                this is turned off, but other VMs may need
> +                                this to be turned on in order to perform at
> +                                all, or might need it if things change.
> +
> +                                Remember, we don't know how many of those
> +                                local refs might have already been used up
> +                                by higher layers of JNI code that end up
> +                                calling g_thread_self(),
> +                                g_thread_set_private(), and so on. */

I think DELETE_LOCAL_REFS should be 1. Always go for the most robust
code.

> +#define  HAVE_JNI_VERSION_1_2   0 /* Assume we don't.  We could
> +                                  dynamically check for this.
> +
> +                                  TODO This code hasn't been tested yet. */

It is time to actually depend on this. Maybe not for 0.10. But lets put
a note in the NEWS file about requiring JNI 1.2 functionality for any
release after that. It makes a couple of things possible/easier in
native code.

> +  /* Pin it down. */
> +  runtimeException_class = (jclass) (*env)->NewGlobalRef (env, lcl_class);
> +  DELETE_LOCAL_REF (env, lcl_class);
> +  if (!runtimeException_class)
> +    {
> +      BADLY_BROKEN1 ("Serious trouble: could not turn"
> +                  " java.lang.RuntimeException into a global reference");
> +      return exception_cache_initialized = -1;
> +    }
> +
> +  assert (runtimeException_class);

This pattern is used so much that it might deserve its own utility
macro/function. But the assert in "if(!x) { ... return ... } assert(x)"
seems unnecessary.

> +/** This is a cache of all classes, methods, and field IDs that we use during
> +   the run.  We maintain a permanent global reference to each of the classes
> +   we cache, since otherwise the (local) jclass that refers to that class
> +   would go out of scope and possibly be reused in further calls.
> +
> +   The permanent global reference also achieves the secondary goal of
> +   protecting the validity of the methods and field IDs in case the classes
> +   were otherwise unloaded and then later loaded again.  Obviously, this will
> +   never happen to classes such as java.lang.Thread and java.lang.Object, but
> +   the primary reason for maintaining permanent global refs is sitll valid.

Note that jnilink.c has a similar objective.

> +
> +/* All of these are cached classes and method IDs: */
> +/* java.lang.Object */
> +jclass obj_class;            /* java.lang.Object */
> +jmethodID obj_ctor;          /* no-arg Constructor for java.lang.Object */
> +jmethodID obj_notify_mth;    /* java.lang.Object.notify() */
> +jmethodID obj_notifyall_mth; /* java.lang.Object.notifyall() */
> +jmethodID obj_wait_mth;              /* java.lang.Object.wait() */
> +jmethodID obj_wait_nanotime_mth; /* java.lang.Object.wait(JI) */
> +
> +/* GThreadMutex and its methods */
> +jclass mutex_class;
> +jmethodID mutex_ctor;
> +jfieldID mutex_lockForPotentialLockers_fld;
> +jfieldID mutex_potentialLockers_fld;
> +
> +/* java.lang.Thread and its methods*/
> +jclass thread_class;         /* java.lang.Thread */
> +jmethodID thread_current_mth;        /* Thread.currentThread() */
> +jmethodID thread_equals_mth; /* Thread.equals() */
> +jmethodID thread_join_mth;   /* Thread.join() */
> +jmethodID thread_setPriority_mth; /* Thread.setPriority() */
> +jmethodID thread_stop_mth;   /* Thread.stop() */
> +jmethodID thread_yield_mth;  /* Thread.yield() */
> +
> +/* java.lang.ThreadLocal and its methods */
> +jclass threadlocal_class;    /* java.lang.ThreadLocal */
> +jmethodID threadlocal_ctor;  /* Its constructor */
> +jmethodID threadlocal_set_mth;       /* ThreadLocal.set() */
> +jmethodID threadlocal_get_mth;       /* ThreadLocal.get() */
> +
> +/* java.lang.Long and its methods */
> +jclass long_class;           /* java.lang.Long */
> +jmethodID long_ctor;         /* constructor for it: (J) */
> +jmethodID long_longValue_mth;        /* longValue()J */
> +
> +
> +/* GThreadNativeMethodRunner */
> +jclass runner_class;
> +jmethodID runner_ctor;
> +jmethodID runner_threadToThreadID_mth;
> +jmethodID runner_threadIDToThread_mth;
> +jmethodID runner_deRegisterJoinable_mth;
> +jmethodID runner_start_mth;  /* Inherited Thread.start() */
> +
> +
> +/* java.lang.InterruptedException */
> +jclass interrupted_exception_class;

If these are only used in this .c file shouldn't they be static?

> +  /* GThreadNativeMethodRunner */
> +  runner_class
> +    = (*env)->FindClass (env,
> +                      "gnu/java/awt/peer/gtk/GThreadNativeMethodRunner");
> +  if ( ! runner_class )
> +    {
> +      BROKEN (env,
> +           "cannot find gnu.java.awt.peer.gtk.GThreadNativeMethodRunner");
> +      return initialized = -1;
> +    }
> +
> +  /* Pin it down. */
> +  runner_class = (jclass) (*env)->NewGlobalRef (env, runner_class);
> +  if (!runner_class)
> +    {
> +      BROKEN (env,
> +           "Cannot get a global reference to the class 
> GThreadNativeMethodRunner");
> +      return initialized = -1;
> +    }
> +  assert (runner_class);

All other class references that are pinned down do an explicit
DELETE_LOCAL_REF. This one should probably also do that.

> +/************************************************************************/
> +/* Miscellaneous utilities                                           */
> +/************************************************************************/
> +
> +/* Get the Java Thread object that corresponds to a particular thread ID. 
> +   A negative thread Id gives us a null object.
> +
> +   Returns a local reference. 
> +*/
> +static jobject
> +getThreadFromThreadID (JNIEnv * env, gpointer gThreadID)
> +{
> +  jint threadNum = (jint) gThreadID;
> +  jobject thread;
> +
> +  if (threadNum < 0)
> +    {
> +      NEW_BROKEN (env,
> +               "getThreadFromThreadID asked to look up a negative thread 
> index");
> +      return NULL;
> +    }
> +
> +  thread = (*env)->CallStaticObjectMethod
> +    (env, runner_class, runner_threadIDToThread_mth, threadNum);
> +
> +  if (MAYBE_BROKEN (env, "cannot get Thread for threadID "))
> +    return NULL;
> +
> +  return thread;
> +}
> +
> +/* Return the unique threadID of THREAD.
> +
> +   Error handling:
> +   Return the pointer equivalent -1 on all failures.
> +
> +   A null thread means we call NEW_BROKEN().
> +   For other errors, return -1 on error and call BROKEN.  */
> +static gpointer
> +getThreadIDFromThread (JNIEnv * env, jobject thread)
> +{
> +  jint threadNum;
> +
> +  if (ENABLE_EXPENSIVE_ASSERTIONS)
> +    assert ((*env)->IsInstanceOf (env, thread, thread_class));
> +
> +  HIDE_OLD_TROUBLE (env);
> +
> +  threadNum = (*env)->CallStaticIntMethod
> +    (env, runner_class, runner_threadToThreadID_mth, thread);
> +
> +  if (MAYBE_BROKEN (env, "cannot get ThreadID for a Thread "))
> +    {
> +      threadNum = -1;
> +      goto done;
> +    }
> +
> +
> +  SHOW_OLD_TROUBLE ();
> +
> +done:
> +  return (gpointer) threadNum;
> +}

Can't we assume that jobject and gpointer are both (void *) so we don't
need the int <-> Thread (global jobject ref) mapping?
Maybe there are platforms where jobject and gpointer aren't the same,
but I guess that is pretty unlikely.

> +/************************************************************************/
> +/* The Actual JNI functions that we pass to the function vector.     */
> +/************************************************************************/

And then we finally come to the real meat of the patch.
But I am so knackered from reading so much C and JNI that I stop here
for now. Sorry.

Lets take a quick look at the java sources.

- Must GThreadMutex be public? It doesn't have any public constructors,
  fields or methods so don't make it public.
- Same for GThreadNativeMethodRunner.
- Use System.arraycopy() for copying arrays.
- threads array will only grow. probably not a problem.
  But we could keep count when nulling entries and shrink when we have
  lots of nulls at the end. Probably not worth it.
- Joinable set. Hmmmm. When calling getThreadIDFromThread() you know
  whether or not the thread is joinable. So just store the Thread itself
  in the threads array? Make that array an Object array and check with
  instanceof. This looks cleaner and more robust to me and it saves a
  native -> java call. But instanceof might be expensive.

And a quick style note. Please also use GNU coding style for the java
source files as you already do for the C code.

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part