Re: [cp-patches] Implementing Thread.sleep() via Thread.wait()

[Mark Wielaard]

Hi,

On Fri, 2004-12-31 at 15:28 -0600, Archie Cobbs wrote:
> Jeroen Frijters wrote:
> >>>+   * A zero length sleep is equivalent to <code>Thread.yield()</code>.
> >>
> >>I think this is not a good idea. This is not supported by any
> >>documentation. And I agree with you that it is probably a bug in the
> >>implementation you tested and filed a bug report for the fact that
> >>sleep(0) seems to ignore the interrupted state of the Thread. 
> >>And if we want to add this bug to our implementation (and I think
> >>we shouldn't) why Thread.yield(), why not just return?
> > 
> > As we apparently can't seem to agree on this, at the very least lets
> > just leave open the option for the VM to do whatever it wants (as I
> > originally proposed).
> 
> What we can do at least is push this "interpretation" down into
> VMThread.sleep() instead of Thread.sleep(), so that if a VM wants
> a different interpretation at least it's easier to implement it.
> 
> Attached is the "final" patch (I hope :-)

I really am not convinced about this "is equivalent to yield()" thing.
If you really want to push this imho broken interpretation then make
sure the mauve tests contain tests for it. Currently they test the
correct behavior of sleep(0) that you mentioned in your bug report about
this issue. Also the documentation of Thread.sleep() should be updated
about this and it should be done clearly to make GNU Classpath
self-contained.

We want to provide an implementation and documentation that is usable
without reference to any interpretations of specific behavior of
non-free proprietary implementation (bugs). And the documentation should
be helpful not only to runtime implementers (which should look in
VMThread) but also (more importantly imho) to developers using the
public classes. So if you really want to push this implementation
document clearly that the InterruptedException is never thrown if all
arguments are zero even if the thread is in interrupted state, that the
interrupted state will/will not (?) be cleared, and that the threading
behavior is like Thread.yield() was called. You can mention that bug
report you files against some other implementation in the implementation
notes/comments if you want, but don't use it in the public description
of the methods.

> +2004-12-31  Archie Cobbs  <address@hidden>
> +
> +     * NEWS, java/lang/Thread.java, vm/reference/java/lang/VMThread.java:
> +     treat Thread.sleep(0) like Thread.yield() for JDK compatibility,
> +     and add a non-native implementation of VMThread.sleep().
> +

An example of how to write this following the hacker guide explanation
of ChangeLog files:

2004-12-31  Archie Cobbs  <address@hidden>

        * NEWS: Add documentation about reference implementation.
        * java/lang/Thread.java (sleep(long,int)): Only do argument
        checking and push implementation to VMThread.sleep().
        * vm/reference/java/lang/VMThread.java (join): Prevent ms
        overflow.
        (sleep): Provide default implementation using Object.wait() and
        Thread.yield().

> --- NEWS        30 Dec 2004 13:18:17 -0000      1.61
> +++ NEWS        31 Dec 2004 20:47:27 -0000
> @@ -17,6 +17,9 @@
>   * VMThread.sleep() will never be called with zero arguments (don't sleep).
>     VMThread does not have to do any extra argument checking. Some (wrong)
>     documentation about the behavior of this method has been updated.
> +  Also, VMThread.sleep() now has a default non-native implementation, but
> +  it is a generic implementation that ignores the nano-seconds argument.
> +  Runtime hackers are encouraged to provide a more efficient version.

You should also mention the join() fixes so runtime implementors are
aware of them.

> -     long end = System.currentTimeMillis() + ms;
> +     // Compute end time, but don't overflow
> +     long now = System.currentTimeMillis();
> +     long end = now + ms;
> +     if (end < now)
> +         end = Long.MAX_VALUE;

Indentation (should be two space). There is more like that, but
apparently the old version already had some wrong indentation issues.
You don't have to fix those now.

> @@ -365,12 +367,51 @@
>       * because some other thread may be active.  So don't expect real-time
>       * performance.
>       *
> -     * @param ms the number of milliseconds to sleep. Will be at least 1.
> +     * <p>
> +     * A zero length sleep is equivalent to <code>Thread.yield()</code>.
> +     * This is simply for compatibility with Sun's JDK. See also
> +     * <a 
> href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6213203";>JDK
> +     * Bug #6213203</a>.
> +     *
> +     * @param ms the number of milliseconds to sleep.
>       * @param ns the number of extra nanoseconds to sleep (0-999999)
>       * @throws InterruptedException if the Thread is (or was) interrupted;
>       *         it's <i>interrupted status</i> will be cleared
>       */
> -    static native void sleep(long ms, int ns) throws InterruptedException;
> +    static void sleep(long ms, int ns) throws InterruptedException

See comments above about documentation of this and Thread.sleep().

I rather not see this patch go in this way (I prefer a default
implementation that does the right thing). But if you really feel
passionately about this and update mauve and the documentation to
reflect this interpretation I won't object.

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part