RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Martin Buchholz]

Using a boolean flag is reasonable, but there are micro-defects here as
well:
- There's a dead initial store to deadline just to appease the definite
assignment algorithm.
- the scope of deadline and remaining leaks into the post-wait-loop code,
and it would be ugly to introduce a new scope explicitly
- if you wait, you will call System.nanoTime() one more time than
necessary.  A high quality wait loop only ever calls nanoTime just before
waiting.

On Mon, Aug 21, 2017 at 12:34 AM, Peter Levart <peter.levart at gmail.com>
wrote:

> Hi Martin,
>
>
> On 08/21/2017 05:05 AM, Martin Buchholz wrote:
>
>> On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz <martinrb at google.com>
>> wrote:
>>
>> Now I see that the code snippet in TimeUnit.timedWait is also in need of
>>> fixing. Hmmmm ....
>>>
>>>   public synchronized Object poll(long timeout, TimeUnit unit)
>>>       throws InterruptedException {
>>>     while (empty) {
>>>       unit.timedWait(this, timeout);
>>>       ...
>>>     }
>>>   }
>>>
>>> It's surprisingly hard to write a high quality wait loop, and we've been
>>>
>> doing our users a disservice by punting on providing good models in the
>> javadoc.  You probably want to work entirely in nanoseconds, to avoid
>> rounding errors, and because you'll end up calling nanoTime anyways.  You
>> don't want to call nanoTime unless you are sure to wait.  OTOH you want to
>> throw NPE on null TimeUnit even if there's no need to wait.  You must
>> beware of overflow when timeout is Long.MIN_VALUE.
>>
>> Below is my attempt to fix the sample loop in timedWait.  Calling
>> Object.wait directly is obviously even harder.  How about just not
>> providing a sample loop that calls Object.wait directly (effectively,
>> gently deprecate timed Object wait in favor of the loop below)  (I'm also
>> pretending that the platform has better than millisecond granularity).
>>
>>   public E poll(long timeout, TimeUnit unit)
>>       throws InterruptedException {
>>     long nanosTimeout = unit.toNanos(timeout);
>>     synchronized (lock) {
>>       for (long deadline = 0L; isEmpty(); ) {
>>         long remaining = (deadline == 0L) ? nanosTimeout
>>           : deadline - System.nanoTime();
>>         if (remaining <= 0)
>>           return null;
>>         if (deadline == 0L) { // first wait
>>           deadline = System.nanoTime() + nanosTimeout;
>>           if (deadline == 0L) // unlikely corner case
>>             deadline = 1L;
>>         }
>>         NANOSECONDS.timedWait(lock, remaining);
>>       }
>>       return dequeueElement();
>>     }
>>   }
>>
>
> You could split the state of "first wait" vs. "deadline" into separate
> locals. It would be easier to understand and no corner case to watch for.
> For example:
>
>     public E poll(long timeout, TimeUnit unit)
>         throws InterruptedException {
>         long nanosTimeout = unit.toNanos(timeout);
>         long deadline = 0;
>         long remaining = nanosTimeout;
>         synchronized (lock) {
>             for (boolean first = true; isEmpty();) {
>                 if (remaining <= 0)
>                     return null;
>                 if (first) { // before first wait
>                     deadline = System.nanoTime() + nanosTimeout;
>                     first = false;
>                 }
>                 NANOSECONDS.timedWait(lock, remaining);
>                 remaining = deadline - System.nanoTime();
>             }
>             return dequeueElement();
>         }
>     }
>
>
> Regards, Peter
>
>