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

[David Holmes]

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;