Patch: assert in natObject.cc
Boehm, Hans
hans_boehm@hp.com
Wed Feb 26 23:24:00 GMT 2003
Looks right to me. I was extremely paranoid about this code, probably with good reason. But it seems to have enough miles on it by now that it's appropriate to turn the assertions off.
Based on my experience, this doesn't have a huge performance impact, in part because the tests probably schedule well, and because the compare-exchange operations are typically so expensive. (If we could save the hash table address from entry to exit somehow, that might make more of a difference, though.)
While we're at it, I attached another patch that has been in my tree for a while. The substantive part of the change is to replace a % with an &. This matters a bit, since the first operand is signed, and thus % involves a test. (An alternative fix would be to make it unsigned; it's a hash function, so it doesn't matter.) If you want to include this in your patch, that's fine. Or I can check it into the trunk separately.
Hans
> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com]
> Sent: Wednesday, February 26, 2003 2:00 PM
> To: java-patches@gcc.gnu.org
> Cc: hans_boehm@hp.com
> Subject: Patch: assert in natObject.cc
>
>
> Anthony pointed out to me that natObject.cc has a lot of assert()
> calls in it, and that these are enabled by default.
>
> Hans, is there a reason not to use JvAssert? This is like assert, but
> we disable it by default. Seems like these asserts would have some
> sort of performance impact.
>
> Tom
>
> Index: ChangeLog
> from Tom Tromey <tromey@redhat.com>
>
> * java/lang/natObject.cc: Don't include assert.h.
> (heavy_lock_obj_finalization_proc): Use JvAssert.
> (remove_all_heavy): Likewise.
> (_Jv_MonitorEnter): Likewise.
> (_Jv_MonitorExit): Likewise.
> (wait): Likewise.
>
> Index: java/lang/natObject.cc
> ===================================================================
> RCS file: /cvs/gcc/gcc/libjava/java/lang/natObject.cc,v
> retrieving revision 1.25
> diff -u -r1.25 natObject.cc
> --- java/lang/natObject.cc 19 Feb 2003 16:28:37 -0000 1.25
> +++ java/lang/natObject.cc 26 Feb 2003 22:05:16 -0000
> @@ -303,7 +303,6 @@
> // that can atomically update only N bits at a time.
> // Author: Hans-J. Boehm (Hans_Boehm@hp.com, boehm@acm.org)
>
> -#include <assert.h>
> #include <limits.h>
> #include <unistd.h> // for usleep, sysconf.
> #include <gcj/javaprims.h>
> @@ -605,7 +604,7 @@
> release_set(&(he -> address), he_address);
> return;
> }
> - assert(hl -> address == addr);
> + JvAssert(hl -> address == addr);
> GC_finalization_proc old_finalization_proc = hl ->
> old_finalization_proc;
> if (old_finalization_proc != 0)
> {
> @@ -653,8 +652,8 @@
> static void
> remove_all_heavy (hash_entry *he, obj_addr_t new_address_val)
> {
> - assert(he -> heavy_count == 0);
> - assert(he -> address & LOCKED);
> + JvAssert(he -> heavy_count == 0);
> + JvAssert(he -> address & LOCKED);
> heavy_lock *hl = he -> heavy_locks;
> he -> heavy_locks = 0;
> // We would really like to release the lock bit here.
> Unfortunately, that
> @@ -664,8 +663,8 @@
> for(; 0 != hl; hl = hl->next)
> {
> obj_addr_t obj = hl -> address;
> - assert(0 != obj); // If this was previously
> finalized, it should no
> - // longer appear on our list.
> + JvAssert(0 != obj); // If this was previously
> finalized, it should no
> + // longer appear on our list.
> hl -> address = 0; // Finalization proc might still
> see it after we
> // finish.
> GC_finalization_proc old_finalization_proc = hl ->
> old_finalization_proc;
> @@ -782,13 +781,13 @@
> if (__builtin_expect(!addr, false))
> throw new java::lang::NullPointerException;
>
> - assert(!(addr & FLAGS));
> + JvAssert(!(addr & FLAGS));
> retry:
> if (__builtin_expect(compare_and_swap(&(he -> address),
> 0, addr),true))
> {
> - assert(he -> light_thr_id == INVALID_THREAD_ID);
> - assert(he -> light_count == 0);
> + JvAssert(he -> light_thr_id == INVALID_THREAD_ID);
> + JvAssert(he -> light_count == 0);
> he -> light_thr_id = self;
> // Count fields are set correctly. Heavy_count was also zero,
> // but can change asynchronously.
> @@ -836,7 +835,7 @@
> // only be held by other threads waiting for conversion, and
> // they, like us, drop it quickly without blocking.
> _Jv_MutexLock(&(hl->si.mutex));
> - assert(he -> address == address | LOCKED );
> + JvAssert(he -> address == address | LOCKED );
> release_set(&(he -> address), (address |
> REQUEST_CONVERSION | HEAVY));
> // release lock on he
> while ((he -> address & ~FLAGS) == (address & ~FLAGS))
> @@ -849,7 +848,7 @@
> // Guarantee that hl doesn't get unlinked by finalizer.
> // This is only an issue if the client fails to release
> // the lock, which is unlikely.
> - assert(he -> address & HEAVY);
> + JvAssert(he -> address & HEAVY);
> // Lock has been converted, we hold the heavyweight lock,
> // heavy_count has been incremented.
> return;
> @@ -866,7 +865,7 @@
> {
> // Either was_heavy is true, or something changed out
> from under us,
> // since the initial test for 0 failed.
> - assert(!(address & REQUEST_CONVERSION));
> + JvAssert(!(address & REQUEST_CONVERSION));
> // Can't convert a nonexistent lightweight lock.
> heavy_lock *hl;
> hl = (was_heavy? find_heavy(addr, he) : 0);
> @@ -879,15 +878,15 @@
> // one first and use that.
> he -> light_thr_id = self; // OK, since nobody else can hold
> // light lock or do this
> at the same time.
> - assert(he -> light_count == 0);
> - assert(was_heavy == (he -> address & HEAVY));
> + JvAssert(he -> light_count == 0);
> + JvAssert(was_heavy == (he -> address & HEAVY));
> release_set(&(he -> address), (addr | was_heavy));
> }
> else
> {
> // Must use heavy lock.
> ++ (he -> heavy_count);
> - assert(0 == (address & ~HEAVY));
> + JvAssert(0 == (address & ~HEAVY));
> release_set(&(he -> address), HEAVY);
> _Jv_MutexLock(&(hl->si.mutex));
> keep_live(addr);
> @@ -898,7 +897,7 @@
> // We hold the lock on the hash entry, and he -> address can't
> // change from under us. Neither can the chain of heavy locks.
> {
> - assert(0 == he -> heavy_count || (address & HEAVY));
> + JvAssert(0 == he -> heavy_count || (address & HEAVY));
> heavy_lock *hl = get_heavy(addr, he);
> ++ (he -> heavy_count);
> release_set(&(he -> address), address | HEAVY);
> @@ -1006,17 +1005,17 @@
> he -> light_count = count - 1;
> return;
> }
> - assert(he -> light_thr_id == self);
> - assert(address & REQUEST_CONVERSION);
> + JvAssert(he -> light_thr_id == self);
> + JvAssert(address & REQUEST_CONVERSION);
> // Conversion requested
> // Convert now.
> if (!compare_and_swap(&(he -> address), address,
> address | LOCKED))
> goto retry;
> heavy_lock *hl = find_heavy(addr, he);
> - assert (0 != hl);
> + JvAssert (0 != hl);
> // Requestor created it.
> he -> light_count = 0;
> - assert(he -> heavy_count > 0);
> + JvAssert(he -> heavy_count > 0);
> // was incremented by requestor.
> _Jv_MutexLock(&(hl->si.mutex));
> // Release the he lock after acquiring the mutex.
> @@ -1033,8 +1032,8 @@
> return;
> }
> // lightweight lock not for this object.
> - assert(!(address & LOCKED));
> - assert((address & ~FLAGS) != addr);
> + JvAssert(!(address & LOCKED));
> + JvAssert((address & ~FLAGS) != addr);
> if (!compare_and_swap(&(he -> address), address, address | LOCKED))
> goto retry;
> heavy_lock *hl = find_heavy(addr, he);
> @@ -1049,9 +1048,9 @@
> throw new java::lang::IllegalMonitorStateException(
> JvNewStringLatin1("current thread not owner"));
> }
> - assert(address & HEAVY);
> + JvAssert(address & HEAVY);
> count = he -> heavy_count;
> - assert(count > 0);
> + JvAssert(count > 0);
> --count;
> he -> heavy_count = count;
> if (0 == count)
> @@ -1160,7 +1159,7 @@
> throw new IllegalMonitorStateException (JvNewStringLatin1
> ("current thread not owner"));
> }
> - assert(address & HEAVY);
> + JvAssert(address & HEAVY);
> }
> switch (_Jv_CondWait (&(hl->si.condition),
> &(hl->si.mutex), timeout, nanos))
> {
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: natObjectdiff
Type: application/octet-stream
Size: 1229 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/java-patches/attachments/20030226/c1fbfa80/attachment.obj>
More information about the Java-patches
mailing list