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

[Ivan Gerasimov]

Thanks David and Daniel!

Yes, as David wrote, a thread-id is 32-bit on Windows.

I'll do all the suggested changes and will push the fix later today.

Sincerely yours,
Ivan

On 06.01.2016 7:25, David Holmes wrote:
> On 6/01/2016 3:33 AM, Daniel D. Daugherty wrote:
>> Happy New Year!
>
> And to you :)
>
>>
>>  > 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
>
> It is a DWORD so 32-bit.
>
> Cheers,
> David
> -----
>
>>          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
>>>>>
>>>>
>>>
>>
>