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

[David Holmes]

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