Review request (S): 7179517 Enable NUMA by default on NUMA hardware

[Igor Veresov]

Looks good!

igor

On Jul 4, 2012, at 8:36 AM, Jesper Wilhelmsson wrote:

> I had to clone a new repository to convince JPRT that I had everything in order, but now the UseNUMA patch passes.
> /Jesper
> 
> 
> On 2012-07-03 17:44, Jesper Wilhelmsson wrote:
>> On 2012-07-03 16:34, Vladimir Kozlov wrote:
>>> On 7/3/12 3:54 AM, Jesper Wilhelmsson wrote:
>>>> Hi Vladimir,
>>>> 
>>>> My IDE wanted me to add os.hpp, I have removed it now.
>>>> I have added UseNUMAInterleaving so that the Solaris patch is identical to
>>>> the linux patch.
>>>> New webrev (same link as before):
>>>> http://cr.openjdk.java.net/~jwilhelm/7179517/webrev/
>>> 
>>> Looks good.
>>> 
>>>> 
>>>> I'm not sure how to handle Solaris though. It won't get the same amount of
>>>> testing as the other platforms. The latest
>>>> patch has not passed trough JPRT yet, I get weird failures. The failures are
>>>> not on Solaris so I don't see how they can
>>>> be related to this change.
>>> 
>>> You should ask when you have problems. It seems, you forgot to clone
>>> src/closed and test/closed into your repository.
>>> 
>> I do have the closed directories in there, could it be that jprt doesn't find
>> them? I'm using -stree when running jprt.
>> I though that I'd get compiler errors rather than a core dump if there were
>> missing files. Is there something in the build process that is dumping core?
>> /Jesper
>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> /Jesper
>>>> 
>>>> 
>>>> On 2012-07-02 21:31, Vladimir Kozlov wrote:
>>>>> Hi Jasper,
>>>>> 
>>>>> Why you added #include "runtime/os.hpp"? It is compiled without it (it is
>>>>> included in an other header file). You would not be able to compile this file
>>>>> since it define methods declared in os.hpp.
>>>>> 
>>>>> UseNUMAInterleaving is set 'true' only for Parallel GC in arguments.cpp. New
>>>>> code set it 'true' for all GCs which may cause problem. Also there is solaris
>>>>> code which depends on this flag and it is not set in this changes.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> Jesper Wilhelmsson wrote:
>>>>>> I have updated the webrev with a Solaris patch as well. It's the same URL,
>>>>>> just do a reload in your browser.
>>>>>> 
>>>>>> I'm not a Solaris native either, so regarding testing I have asked the
>>>>>> performance team to look extra close on Solaris to make sure it behaves as
>>>>>> expected.
>>>>>> /Jesper
>>>>>> 
>>>>>> 
>>>>>> On 2012-07-02 19:26, Vladimir Kozlov wrote:
>>>>>>> That question was not for you but for Jesper.
>>>>>>> 
>>>>>>> Vladimir
>>>>>>> 
>>>>>>> Eric Caspole wrote:
>>>>>>>> I am hoping one of you guys can do it - I have never used Solaris and I
>>>>>>>> don't know if there is any special thing to take into account there.
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Jul 2, 2012, at 12:07 PM, Vladimir Kozlov wrote:
>>>>>>>> 
>>>>>>>>> Why there is no changes for Solaris?
>>>>>>>>> 
>>>>>>>>> Vladimir
>>>>>>>>> 
>>>>>>>>> On 7/2/12 3:57 AM, Jesper Wilhelmsson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>> 
>>>>>>>>>> This change is to turn on UseNUMA per default on NUMA hardware. It has
>>>>>>>>>> been contributed by Eric Caspole. I'm fine with
>>>>>>>>>> the change but we need at least one official reviewer.
>>>>>>>>>> 
>>>>>>>>>> Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~jwilhelm/7179517/webrev/
>>>>>>>>>> 
>>>>>>>>>> Testing:
>>>>>>>>>> I ran it through JPRT and did some local sanity testing.
>>>>>>>>>> Eric has tested this on SPECjbb2005 and SPECjvm2008 with a variety of
>>>>>>>>>> heap
>>>>>>>>>> sizes and it is never worse than the current
>>>>>>>>>> default. In some cases, especially with Windows, running a test with the
>>>>>>>>>> current default and getting bad NUMA placement
>>>>>>>>>> will be about 3x slower than the +UseNUMA score.
>>>>>>>>>> 
>>>>>>>>>> The performance team are involved to perform some larger scale testing.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>