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

[Igor Ignatyev]

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
>>>>>>
>>>>
>>