Re: portable-native-sync-7.patch

[Mark Wielaard]

Hi Steven,

Thanks again for all your hard work.
The patch has already seen great use by other projects so lets finally
check this in. The current code is broken. And we can go back and forth
over some smaller style issues, but I believe it is almost perfect
already and this patch is just to big to get all tiny thingies all right
(especially since the original code wasn't really written in out current
coding style guide).

For the future try to split such a big patch in:
- Reformatting.
- Introduce helper methods/macros in old code.
- New implementation.
That makes reviewing so much easier.

Some small suggestions only. I am impressed how you get java thread
semantics and glib thread semantics as close as possible.

Do the ChangeLog like this (note removal of large number of newlines and
placing of the colon):

2004-05-26  Steven Augart  <address@hidden>

        --portable-native-sync implemented for GTK2.
        * gnu/native/jni/gtk-peer/gthread-jni.c: Indentation fixes.
        Implemented missing functions for GTK2.
        Added error handling.
        Renamed static functions out of the g_ namespace.
        Added TRACE_API_CALLS, EXPLAIN_TROUBLE, EXPLAIN_BROKEN, 
        EXPLAIN_BADLY_BROKEN, and DELETE_LOCAL_REFS options.
        Rewrote global-reference code.
        Eliminated cascading errors.
        (mutex_trylock_jni_impl) Fully implemented.
        (cond_timed_wait_jni_impl) Went from millisecond to microsecond
        resolution.
        (setup_cache) New function.
        (mutex_cond_wait_jni_impl, mutex_cond_timed_wait_jni_impl) Fixed
        bug where they were not unlocking the GMutex associated with the
        condition variable during the wait on that condition variable.

        * native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c,
        native/jni/gtk-peer/gthread-jni.c,
        native/jni/gtk-peer/gthread-jni.h
        (g_thread_jni_functions): Renamed to ...
        (portable_native_sync_jni_functions): this name.
        (gdk_vm): Renamed to...
        (the_vm): this name.
        
        * native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkMainThread.c
        (gdk_vm): Removed duplicate definition.
        (gtkInit): Removed stray message to stdout.
        (gtkInit): Use g_malloc and g_free instead of malloc and free.
        (gtkInit): Fix a const assignment bug.
        (gtkInit): Simplified code.
        
        * gnu/java/awt/peer/gtk/GThreadNativeMethodRunner.java,
        native/jni/gtk-peer/gnu_java_awt_peer_gtk_GThreadNativeMethodRunner.c,
        native/jni/gtk-peer/gnu_java_awt_peer_gtk_GThreadNativeMethodRunner.h,
        gnu/java/awt/peer/gtk/GThreadMutex.java:
        New files.

[...]
+class GThreadNativeMethodRunner 
+  extends Thread 
+{
[...]
+  GThreadNativeMethodRunner(long funcPtr, long funcArg, boolean joinable) 
+  {
+    this.funcPtr = funcPtr;
+    this.funcArg = funcArg;
+//     this.myThreadID = registerThread(self); // register self numerically
+    if (joinable)
+      registerSelfJoinable();
+  }

Two comments here.
Maybe call super(String) to set a thread name for debugging purposes.
The commented out code looks confusing (even though there is a comment
at the top.

[several places]
+  (*the_vm)->GetEnv (the_vm, (void **) &env, JNI_VERSION_1_1);

Interesting enough according to the JNI book GetEnv is actually a JNI
1.2 extension. But strangely it does accept the JNI_VERSION_1_1
interface id. 

So lets document somewhere that we do need this JavaVM interface.
Since I don't see any way we can do this with real plain JNI 1.1.

+    The glib library is really designed so that you should fail
+    catastrophically in case of "programming errors".  But they don't seem to
+    have thoroughly considered cases like resource exhaustion, at least not
+    with respect to these functions.  The only error defined for the GThread
+    functions is G_THREAD_ERROR_AGAIN, and that for thread_create.  Most of 
these GThread functions could
+    fail if we run out of memory, for example.  And if we run out of Java
+    memory, we are almost certainly not going to get any more.  Never the
+    less, it does not seem appropriate for 
+*/

This comment doesn't end?
IMHO G_THREAD_ERROR_AGAIN is appropriate for out of memory situations
or other resource issues.

--- NEWS        2 May 2004 13:47:32 -0000       1.39
+++ NEWS        26 May 2004 21:05:00 -0000
@@ -1,3 +1,13 @@
+New in release 0.10 (in preparation)
+
+* Java primitives can once again be used to support AWT native
+  threading; this had been broken since Classpath release 0.06, when
+  we upgraded to GTK+2.  This is enabled with the
+  --enable-portable-native-sync configure option.

Maybe explicitly say that GTK+ normally uses the platform native
threading model (pthreads in most cases) and if the choosen runtime
doesn't use the same threading model then you need this option.

+* We intend that the next release of GNU Classpath will require the VM 
+  to provide JNI 1.2.  If there are serious problems with this, please
+   raise them now. 

Lets post a RFC about this also on the main mailinglist.
Plus the GetEnv() remark above.

Thanks,

Mark

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