commit-classpath
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: portable-native-sync-5.patch


From: Steven Augart
Subject: Re: portable-native-sync-5.patch
Date: Wed, 26 May 2004 13:56:10 -0400
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040421

Dear Mark,

Mark Wielaard wrote:

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.

I should have read that part of the hacking guide better. Since the code I started with already did its own error handling and exception reporting, I just adapted it further, instead of looking for
ways to use the existing infrastructure.

I've added the comments to Chapter 8 and have submitted them as hacking-1.patch.


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.

I agree.  I don't want to make a second patch to do this until this
one is committed into the system.  I think the functionality will be
trivial to add.

I've changed the comment appropriately.

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

Thanks.  I will consider that.  I'll have to see how the build behaves
with them.
[....]

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

Good point.  Changed.

I also have gone over the code again and found that I wasn't explicitly deleting local references to Threads. Fixed.

+#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.

Is this absolutely a good idea, given that our nominal target is the 1.1 JDK API? There is no way that requirement could make a VM fail to meet the Java 1.1 compatibility tests, is there? (I added a NEWS item for it in any case, to excite comment.)

+  /* 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.

That makes sense and might have been better. It would be a bit more robust; as you pointed out, at one place I'd neglected to free the global reference.

But the assert in "if(!x) { ... return ... } assert(x)"
seems unnecessary.

I've removed all of the assertions that follow this pattern.

+/** 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.

Comment added.

+
+/* All of these are cached classes and method IDs: */
[...]
If these are only used in this .c file shouldn't they be static?

Whoops.  Fixed.

[...]

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

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.

Don't be.  Thank you for the thoughtful feedback.

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.
Changed. Since JNI side-steps Java access checks, do you want the fields to be declared private, too?

- Use System.arraycopy() for copying arrays.
Changed.

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

I have added your comment to the sources, attributed to you.

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

I have added this comment to the sources.

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

Done.

Once I've tested that the code still works with these changes, I will submit portable-native-sync-6.patch to commit-classpath. That patch will also contain a new ChangeLog entry, as you and Tom and I discussed on #classpath.

Cordially Yours,

--Steven Augart

--
Steven Augart

Jikes RVM, a free, open source, Virtual Machine:
http://oss.software.ibm.com/jikesrvm





reply via email to

[Prev in Thread] Current Thread [Next in Thread]