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

[Lev Priima]

Thanks Vladimir,

Lev

On 06/03/2014 09:36 PM, Vladimir Kozlov wrote:
> 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
>>>>>>>>>
>>>>>>>
>>>>>
>>