RFR(integration blocker): 8191373: Multiple NUMA nodes expected

[Robbin Ehn]

Thank you David for taking the time and doing a thorough review.

/Robbin

On 2017-11-20 05:28, David Holmes wrote:
> Hi Robbin,
> 
> tl;dr: looks good. reviewed.
> 
> So with TLH you introduced SafepointMechanism::initialize() and inserted it into the init sequence here:
> 
> +  SafepointMechanism::initialize();
> +
>     // Initialize the os module after parsing the args
>     jint os_init_2_result = os::init_2();
> 
> so prior to os::init_2(). Most (all?) of the code in SafepointMechanism::initialize() was actually factored out from the platform specific os::init_2() functions. But in doing the factoring you replaced native OS calls like VirtualAlloc with more abstract os::* APIs. So now you have a method that depends on various os::* APIs but which is called before os initialization is fully complete. And it turns out on Windows those functions depended on something only done in os::init_2(), so you've now moved the initialization function to after os::init_2().
> 
> So as long as there is nothing in os::init_2() that may depend on the initialization now done afterwards via SafepointMechanism::initialize(), this should be a good fix - and handle any other unforeseen init_2() dependencies that may not have surfaced.
> 
> As should not be encountering safepoints or handshakes until well after this point in the init sequence this all seems okay to me.
> 
> Thanks,
> David
> 
> On 19/11/2017 10:34 PM, David Holmes wrote:
>> Hi Robbin,
>>
>> I need to study this more closely. Need to check the original code before the TLH code, to evaluate impact.
>>
>> Initialization order issues are rarely simple to fix as there are so many hidden dependencies.
>>
>> Will get back to you asap.
>>
>> David
>>
>> On 18/11/2017 12:10 AM, Robbin Ehn wrote:
>>> Hi all, please review.
>>>
>>> Windows needs to run os::init_2 before allocation polling page to proper setup numa. I saw no reason not to have it for all platforms after init_2.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191373
>>>
>>> Code below.
>>>
>>> Tested tier 1-5/1-3
>>>
>>> Thanks, Robbin
>>>
>>> diff -r b4d2929683b6 src/hotspot/share/runtime/thread.cpp
>>> --- a/src/hotspot/share/runtime/thread.cpp    Fri Nov 17 02:50:51 2017 +0100
>>> +++ b/src/hotspot/share/runtime/thread.cpp    Fri Nov 17 13:30:28 2017 +0100
>>> @@ -3560,10 +3560,10 @@
>>>     TraceTime timer("Create VM", TRACETIME_LOG(Info, startuptime));
>>>
>>> -  SafepointMechanism::initialize();
>>> -
>>>     // Initialize the os module after parsing the args
>>>     jint os_init_2_result = os::init_2();
>>>     if (os_init_2_result != JNI_OK) return os_init_2_result;
>>>
>>> +  SafepointMechanism::initialize();
>>> +
>>>     jint adjust_after_os_result = Arguments::adjust_after_os();
>>>     if (adjust_after_os_result != JNI_OK) return adjust_after_os_result;