RFR 8145127: VM warning: WaitForMultipleObjects timed out (0) ...

[Daniel D. Daugherty]

Happy New Year!


 > http://cr.openjdk.java.net/~igerasim/8145127/01/webrev/

src/os/windows/vm/os_windows.cpp
     L3923:         Atomic::cmpxchg((jint)GetCurrentThreadId(), 
&process_exiting, 0);
         What's the return size of GetCurrentThreadId()? Are we
         down casting a larger size into a 'jint'? If so, that
         might allow more than one thread into this code when we
         only want one...

         Also please consider adding a comment above this line.
         Perhaps something like:

         // Atomically set process_exiting before the critical section
         // to increase the visibility between racing threads.

     L3998:             if (portion_count > MAXIMUM_WAIT_OBJECTS) {
     L3999:                 portion_count = MAXIMUM_WAIT_OBJECTS;
         Wrong indent; should be two spaces relative to L3998.

     L4013:               portion_count = handle_count - i;
         Please consider adding a comment for this error case.
         Perhaps something like:
         // Reset portion_count so we close the remaining
         // handles due to this error.

     L4030:     if (OrderAccess::load_acquire(&process_exiting) != 0 &&
     L4031:         process_exiting != (jint)GetCurrentThreadId()) {
     L4032:       while (true) {
     L4033:         // Some other thread is about to call exit(), so we
     L4034:         // don't let the current thread proceed to exit() or 
_endthreadex()
         The comment on L4033-4 is slightly misplaced now. It
         should be between L4031 and L4032.


Thumbs up modulo the GetCurrentThreadId() return size question
above. If that size is OK and you choose to make the comment
changes, I don't need to see another webrev.

Dan


On 12/23/15 5:20 AM, Ivan Gerasimov wrote:
> Thank you David for review!
>
> Sincerely yours,
> Ivan
>
> On 23.12.2015 7:41, David Holmes wrote:
>> Looks okay.
>>
>> Second review needed though.
>>
>> Thanks,
>> David
>>
>> On 19/12/2015 10:28 PM, Ivan Gerasimov wrote:
>>>
>>>
>>>>> We will suspend the current thread if two conditions are satisfied:
>>>>> process_exiting != 0 and process_exiting != current thread id.
>>>>> But, yes, pr_ex isn't really needed, as we can use process_exiting
>>>>> directly.
>>>>>
>>>>> Here's the updated webrev:
>>>>> http://cr.openjdk.java.net/~igerasim/8145127/01/webrev/
>>>>
>>>> I still see pr_ex ??
>>>>
>>>
>>> Oops.
>>> Forgot to 'hg qrefresh' before generating the webrev.
>>> The webrev has just been updated in place.
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>
>