harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Weldon Washburn" <weldon...@gmail.com>
Subject Re: Re: [drlvm][threading] is harmony-1951 a bug or a feature?
Date Wed, 22 Nov 2006 01:16:45 GMT
On 11/21/06, Elford, Chris L <chris.l.elford@intel.com> wrote:
>
> Hi all,
>
> Since Salikh brings up the subject of spurious wakeups, I'm not sure
> that any of these tests are written to adequately avoid hangs on all
> possible compliant VM implementations.
>
> The way I read test3.java, if the meToWait thread gets a spurious wakeup
> to Test2.lock.wait() before main has finished its initial synchronized
> block, it is possible that the synchronized block in main will never
> complete as it will never be able to get the lock back from the
> meToWaitThread.


Interesting point.  To test this theory, I put a,
"System.out.println("sleep(10)
just completed");" immediately after the "sleep(10);" in the method called
run().  On my laptop, DRLVM executes the println statement endlessly.
However, the main thread has printed, "Both threads are waiting now, will
interrupt the second one".  This indidates the main thread has already
entered/exited the synchronized(Test2.lock) {...}.   Hmmm...

I then added another print statement,  "System.out.println("Just did
Test2.toStop.interrupt();"   Of course this new line is immedately after
Test2.toStop.interrupt();  It turns out this print statement appears which
suggests the main thread is sending the interrupt and the child thread is
somehow missing it.


Now one could correct for that by replacing the
> sleep(10) with a Test2.lock.wait(10).  Or if one wanted a less subtle
> testcase, putting all the wait() calls into loops as suggested by the
> java/lang/Object API specification.



I tried replacing sleep(10); with Test2.lock.wait();  The app no longer
hangs, but the above printf's indicate there may still be a bug(s) in how
drlvm handles interrupts.

In general, I agree that spurious wakeups are something to be avoided by
> the implementation where possible [personally, I'd like to see them
> disallowed].  However, given that the spec allows spurious wakeups, it
> could actually be a valuable development tool to have an option to
> insert them extensively to help developers identify places where they
> have inadequately protected themselves against true spurious wakeups.
> Therefore, I could see this partially as a feature.


Yes!  I like this idea.  It should not be too hard to add to drlvm when the
time is right.

Having said that, I support the default behavior for an implementation
> to notify only the target thread instead of all threads.


I agree.  If for no other reason than to reduce confusion.  If at a later
time we discover this "feature" is causing unacceptable performance
problems, we can always yank it or put it command line selectable.

Chris
>
> -----Original Message-----
> From: news [mailto:news@sea.gmane.org] On Behalf Of Salikh Zakirov
> Sent: Tuesday, November 21, 2006 10:41 AM
> To: dev@harmony.apache.org
> Subject: Re: [drlvm][threading] is harmony-1951 a bug or a feature?
>
> Artem Aliev wrote:
> > Guys,
> >
> > I attach a kind of fix for the problem to the JIRA, but I'm not sure I
> > want to commit such fix.
> > Please review and comment
>
> Thanks to Artem for clear explanation of the behaviour of the Test3.
> (see HARMONY-1951 comments).
>
> Formally speaking, the Java specification does not guarantee that
> Test3.java
> from HARMONY-1951 will work correctly, as specification allows for
> "spurious" wake ups from wait().
>
> Thus, the behaviour of the test is still within compliant with the spec.
>
> Still, I agree that it would be nice to fix interruption() to prevent
> spurious wakeups.
>
> Regarding the fix proposed by Artem, I think it is overly complex,
> and does many things that are not required by the specification.
> I'm thinking about a simpler fix using a state variable in
> HyThreadMonitor.
> (which is conveniently protected by monitor mutex)
> It looks like it fixes the Test3, but I need to test it some more.
>
> diff --git vm/thread/src/thread_native_fat_monitor.c
> vm/thread/src/thread_native_fat_monitor.c
> index 7e812a2..9994c22 100644
> --- vm/thread/src/thread_native_fat_monitor.c
> +++ vm/thread/src/thread_native_fat_monitor.c
> @@ -67,6 +67,7 @@ IDATA VMCALL hythread_monitor_init_with_
>     mon->flags = flags;
>     mon->name  = name;
>     mon->owner = 0;
> +    mon->notify_flag = 0;
>
>         *mon_ptr = mon;
>     return TM_ERROR_NONE;
> @@ -205,8 +206,33 @@ IDATA monitor_wait_impl(hythread_monitor
>     self->state |= TM_THREAD_STATE_IN_MONITOR_WAIT;
>     hymutex_unlock(self->mutex);
>
> -    status =  condvar_wait_impl(mon_ptr->condition, mon_ptr->mutex, ms,
> nano, interruptable);
> -
> +    do {
> +        apr_time_t start;
> +        mon_ptr->notify_flag = 0; // clear flag before waiting
> +        start = apr_time_now();
> +        status = condvar_wait_impl(mon_ptr->condition, mon_ptr->mutex,
> ms, nano, interruptable);
> +        if (status != TM_ERROR_NONE
> +                || mon_ptr->notify_flag || hythread_interrupted(self))
> +            break;
> +        // we should not change ms and nano if both are 0 (meaning "no
> timeout")
> +        if (ms || nano) {
> +            apr_interval_time_t elapsed;
> +            elapsed = apr_time_now() - start; // microseconds
> +            nano -= (IDATA)((elapsed % 1000) * 1000);
> +            if (nano < 0) {
> +                ms -= elapsed/1000 + 1;
> +                nano += 1000000;
> +            } else {
> +                ms -= elapsed/1000;
> +            }
> +            if (ms < 0) {
> +                assert(status == TM_ERROR_NONE);
> +                status = TM_ERROR_TIMEOUT;
> +                break;
> +            }
> +            assert(0 <= nano && nano < 1000000);
> +        }
> +    } while (1);
>     hymutex_lock(self->mutex);
>     self->state &= ~TM_THREAD_STATE_IN_MONITOR_WAIT;
>     self->current_condition = NULL;
> @@ -321,6 +347,7 @@ IDATA VMCALL hythread_monitor_notify_all
>         mon_ptr->notify_flag=1;
>         return TM_ERROR_NONE;
> #else
> +    mon_ptr->notify_flag = 1;
>         return hycond_notify_all(mon_ptr->condition);
> #endif
> }
> @@ -347,6 +374,7 @@ IDATA VMCALL hythread_monitor_notify(hyt
>         mon_ptr->notify_flag=1;
>         return TM_ERROR_NONE;
> #else
> +    mon_ptr->notify_flag = 1;
>         return hycond_notify(mon_ptr->condition);
> #endif
> }
>



-- 
Weldon Washburn
Intel Enterprise Solutions Software Division

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message