Re: [Sablevm-developer] branch permissions and tag creation

[Chris P. <chr...@ma...>]

Chris Pickett wrote:
> As it stands, it's a pretty benign 
> patch, and I don't really see any problem with it (otherwise I would not 
> have suggested it for staging).

I just found that on tofu, a 2-processor SMP machine, that this change 
can cause a NullPointerException if you run a simple Incrementer.java 
benchmark enough times (attached to list some time ago).  I did not 
observe this on other machines.

However, I do not think this is a problem with my change, because all it 
does is inflate an already held lock.  Please don't interpret this as 
arrogance; I honestly don't see why the change itself would be 
responsible for this behaviour.  All I can think of is that it is 
exposing an existing problem with the locking implementation (and indeed 
I was seeing several NullPointerException problems earlier on this 
year).  Putting memory barriers in the JSR-133 cookbook-specified places 
does not fix it.  Relevant changes from the tag are attached for those 
interested without easy repository access (e.g. Clark).

I suppose we probably don't want to commit it now ... but I would 
appreciate at some point trying to nail this problem down (possibly 
meeting in person).

Cheers,
Chris

Index: src/libsablevm/thread.c
===================================================================
--- src/libsablevm/thread.c     (.../sablevm/branches/staging) 
(revision 2150)
+++ src/libsablevm/thread.c 
(.../developers/chris/tags/inflate_thin_locks)  (revision 2150)
@@ -949,6 +949,12 @@
    _svmt_JavaVM *vm = env->vm;
    _svmt_word old_lockword;

+#ifdef _SABLEVM_FULL_LOCK_INFLATION
+
+ retry:
+
+#endif /* _SABLEVM_FULL_LOCK_INFLATION */
+
    assert (instance != NULL);

    old_lockword = instance->lockword;
@@ -971,9 +977,39 @@

        if (recursive_count == 0)
         {
-         /* we're releasing the thin lock */
-         instance->lockword = _svmf_lockword_get_extra_bits (old_lockword);
-         goto handle_contention;
+
+#ifdef _SABLEVM_FULL_LOCK_INFLATION
+
+         /* CJFP
+            if the thin lock being released is under contention,
+            we now inflate it to avoid taking the slow inflation
+            path again.
+         */
+
+         if (env->contention.owner.flag)
+           {
+             if (_svmf_inflate_lock_no_exception (env, instance) != JNI_OK)
+               {
+                 _svmf_error_OutOfMemoryError (env);
+                 return JNI_ERR;
+               }
+             goto retry;
+           }
+         else
+
+#endif /* _SABLEVM_FULL_LOCK_INFLATION */
+
+           /* EMG
+              we're releasing the thin lock
+
+              CJFP
+              equivalent to line 22 in Onodera's paper
+           */
+
+           {
+             instance->lockword = _svmf_lockword_get_extra_bits 
(old_lockword);
+           }
+         goto handle_contention;
         }
        else
         {