[9] RFR (XS) 8044339: wrong comment for FilterSpuriousWakeups option

[Lev Priima]

Thanks David and Igor!
Anyway, I think it's easier to find a second reviewer, if he does not 
have to be a sponsor. Maybe someone will review this update:

http://cr.openjdk.java.net/~iignatyev/lpriima/8044339/webrev.03/

Lev

On 06/03/2014 11:56 AM, Igor Ignatyev wrote:
> Lev,
>
> 1. You have a typo in 'src/os/linux/vm/os_linux.cpp':
>> 5541   // We We ignore spurious OS wakeups unless 
>> FilterSpuriousWakeups is false.
>                ^
> 2. According to the process[1], you need to write a regression test or 
> add a 'noreg-*' label to the bug. I think adding 'noreg-doc' is a good 
> choice.
>
> Otherwise, your changes look good to me. I'm not a Reviewer, but I 
> doubt that such changes require review from two Reviewers, so if 
> there's no objections, I'll sponsor your changes.
>
> [1] http://openjdk.java.net/guide/changePlanning.html
>
> Thanks
> Igor
>
> On 06/03/2014 06:26 AM, David Holmes wrote:
>> Looks good to me!
>>
>> You'll need a second reviewer and a sponsor.
>>
>> Thanks,
>> David
>>
>> On 2/06/2014 7:22 PM, Lev Priima wrote:
>>> Thanks, please review update:
>>> http://cr.openjdk.java.net/~iignatyev/lpriima/8044339/webrev.02/
>>>
>>> Lev
>>>
>>> On 06/02/2014 07:43 AM, David Holmes wrote:
>>>> Hi Lev,
>>>>
>>>> On 31/05/2014 1:57 AM, Lev Priima wrote:
>>>>> Thanks for comments, please look at update:
>>>>> http://cr.openjdk.java.net/~iignatyev/lpriima/8044339/webrev.01/
>>>>
>>>> Please generalize the CR summary and description if you are going to
>>>> pursue all these changes. But include additional information to
>>>> clarify why you made the changes in the os_XXX.cpp files please. On
>>>> that note can you please change:
>>>>
>>>> !   // We must filter out spurious wakeups.
>>>>
>>>> to
>>>>
>>>> !   // We ignore spurious OS wakeups unless FilterSpuriousWakeups is
>>>> false
>>>>
>>>> In globals.hpp: Note that CompilerThreadHintNoPreempt and
>>>> VMThreadHintNoPreempt are actually only meaningful on Solaris. They
>>>> guard the call to os::hint_no_preempt which is a no-op except on
>>>> Solaris. So those changes should be reverted.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Lev
>>>>>
>>>>> On 05/30/2014 10:03 AM, David Holmes wrote:
>>>>>> Hi Lev,
>>>>>>
>>>>>> On 30/05/2014 4:33 AM, Lev Priima wrote:
>>>>>>> Please review and help me with integration:
>>>>>>>
>>>>>>> diff -r 102506d9d873 src/share/vm/runtime/globals.hpp
>>>>>>> --- a/src/share/vm/runtime/globals.hpp    Wed May 28 14:42:00 2014
>>>>>>> +0400
>>>>>>> +++ b/src/share/vm/runtime/globals.hpp    Thu May 29 22:29:37 2014
>>>>>>> +0400
>>>>>>> @@ -1144,7 +1144,7 @@
>>>>>>> \
>>>>>>>     product(bool, FilterSpuriousWakeups,
>>>>>>> true,                                \
>>>>>>>             "Prevent spurious or premature wakeups from object.wait
>>>>>>> "         \
>>>>>>> -          "(Solaris
>>>>>>> only)") \
>>>>>>> +          "(Ignored for
>>>>>>> Windows)")                                          \
>>>>>>> \
>>>>>>
>>>>>> Sorry to make life complicated but as I put in the bug report:
>>>>>>
>>>>>> The "Solaris" part dates from the days when Solaris meant "not
>>>>>> Windows". :)
>>>>>>
>>>>>> A bigger problem here is that the main part of the description:
>>>>>>
>>>>>> "Prevent spurious or premature wakeups from object.wait "
>>>>>>
>>>>>> may not match what people expect - it depends on whether "filter"
>>>>>> means to remove the item being filtered, or to keep only the item
>>>>>> being filtered. It would clearer if it read "When true prevents ..."
>>>>>> but even then it is not accurate as this only excludes spurious
>>>>>> wakeups from the OS pthread_cond[timed]wait implementation - the JVM
>>>>>> can still induce its own "spurious wakeups". So the better 
>>>>>> description
>>>>>> would be:
>>>>>>
>>>>>> "When true prevents OS-level spurious, or premature, wakeups from
>>>>>> Object.wait"
>>>>>>
>>>>>> But it should also be noted that where this flag is checked there is
>>>>>> an even more inaccurate comment:
>>>>>>
>>>>>>   // The underlying Solaris implementation, cond_timedwait, admits
>>>>>>   // spurious/premature wakeups, but the JLS/JVM spec prevents the
>>>>>>   // JVM from making those visible to Java code.
>>>>>>
>>>>>> Ironically this comment appears in the linux and bsd versions but 
>>>>>> not
>>>>>> the Solaris one! Further it is incorrect - the JLS/JVMS does not
>>>>>> prevent spurious wakeups from being made visible to Java code. That
>>>>>> has always been the intent but it took until Java 5 before we got 
>>>>>> the
>>>>>> specification for Object.wait updated to make it clear.
>>>>>>
>>>>>> So really this is all legacy crud that should be removed along with
>>>>>> the other planned cleanups (ie removing the safe_cond_* functions 
>>>>>> and
>>>>>> WorkAroundNPTLTimedWaitHang) and the os_posix refactoring.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Sorry to mess up your "starter" bug.
>>>>>>
>>>>>> David
>>>>>>
>>>>>>
>>>>>>>     product(intx, NativeMonitorTimeout, -1,
>>>>>>> "(Unstable)")                     \
>>>>>>> \
>>>>>>>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8044339
>>>>>>>
>>>>>
>>>