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

[Vladimir Kozlov]

Looks good.

Vladimir

On 6/3/14 7:25 AM, Lev Priima wrote:
> 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
>>>>>>>>
>>>>>>
>>>>
>