harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Rana Dasgupta" <rdasg...@gmail.com>
Subject Re: [drlvm][threading] is harmony-1951 a bug or a feature?
Date Wed, 22 Nov 2006 01:02:05 GMT
Hi,
  The test is a simple one and only a small extension of Geir's case. All it
tries to do is to force the releases to be in the order 1) toStop thread 2)
toWait thread and fail otherwise. My impression is that this was the intent
of the original test, that's the order a normal app developer would expect,
and that's the release order in RI. The fact that perfect java programs
should do all waits in conditional loops, or that Test2.lock.wait(xx) yields
by releasing locks and sleep() or yield() yields retaining locks is all
quite true. But the purpose of this test and its modification was specific.
  The spec does mention the possibility of spurious wakeups and the
application coding to protect against it, but I don't think that it
recommends it as a JVM feature. As I mentioned, there is also nothing
preventing the optimized compiled code from moving things around and  the
interrupt being issued to the toWait thread before being issued to the
toStop thread. If it does this, even the RI would hang( as can be seen by
moving the toWait.interrupt() before the toStop.interrupt() in the test ).
Assuming a time order of acquisition/release in concurrent code is not a
healthy idea anyway.
  I don't think that DRLVM violates the spec here. The test ,if perfectly
written, would not cause a hang. And we can choose not to change anything.
But I don't think that implementing thread.interrupt() with an
unconditional notifyAll is a good idea. Salikh's change looks simple enough.
But needs to be tested for all kinds of races. I also hope that it will not
have scaling issues for a very large number of threads.

Thanks,
Rana



On 11/21/06, Salikh Zakirov <Salikh.Zakirov@intel.com> wrote:
>
> 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
> }
>
>

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