RFR (S): 7198334: UseNUMA modifies system parameters on non-NUMA system

[Erik Helin]

Hi David,

thank you for reviewing this change!

Please see an updated webrev at:

http://cr.openjdk.java.net/~mgerdin/JDK-7198334/

I've responded inline.

On 10/30/2012 04:29 AM, David Holmes wrote:
> On 30/10/2012 12:29 AM, Erik Helin wrote:
>> Since 'Arguments::parse' is called before 'os::init_2' in the function
>> 'Threads::create_vm', the result was that 'UseNUMA' was set to false and
>> 'UseNUMAInterleaving' and 'MinHeapDeltaBytes' were incorrectly
>> modified.
>
> Could we not simply adjust UseNUMAInterleaving and MinHeapDeltaBytes at
> the same time as setting UseNUMA to false? Or move the setting of
> UseNUMA to false out of os::init_2 into an earlier function - os::init
> perhaps?

We could adjust UseNUMAInterleaving and MinHeapDeltaBytes in os::init_2 
which is responsible for setting UseNUMA to false. However, I don't 
think that os should adjust MinHeapDeltaBytes, which is a GC flag.

We can not move the setting of UseNUMA to os::init, because we don't 
want to try to load libnuma (on Linux) if UseNUMA is not set to true, 
which means that UseNUMA has to be parsed before os can check if it 
should load libnuma.

On 10/30/2012 04:29 AM, David Holmes wrote:
> I don't understand the comment about UseLargePages. But what you have
> done to deal with this single UseNUMA issue is not only move that piece
> of code (which is inside set_parallel_gc_flags()) but all of the GC flag
> setting that previously occurred *before* os::init_2() was called.
> Further you now have to verif that none of the option management that
> now happens before the GC flags are set in any way relies upon the
> settings of those GC flags!
>
> This just seems too disruptive and difficult to test adequately.

I agree, I moved too much functionality into Arguments::adjust_after_os.

I do not think that simply splitting Arguments::parse into two 
functions, Arguments::parse and Arguments::adjust_before_os is a 
disruptive change if Arguments::adjust_before_os ends up being called 
directly after Arguments::parse. In fact, this change is not needed, but 
I think it makes the code easier to understand.

With the new change, which only moves the check for UseNUMA into 
Arguments::ajdust_after_os, I don't think that this change is too 
disruptive either.

Please see the new webrev for more details.

On 10/30/2012 04:29 AM, David Holmes wrote:
>> This change also renames 'os::init' to 'os::init_before_parsing_flags'
>> and 'os::init_2' to 'os::init_after_parsing_args'. This was done to try
>> to make the relationship between the functions in 'Arguments' and the
>> functions in 'os' as clear as possible.
>
> A little too verbose though I think. Maybe os::init_pre_args() and
> os::init_post_args() ? Though really the names are not that significant
> given the comments both at the call site and declaration site of the
> methods.

My idea was that the code should "speak for itself" and hopefully render 
the comments unnecessary, since comments sometimes can become outdated 
but code can't.

My idea was actually to remove the comments and just keep the verbose 
names, what do you think of this?

On 10/30/2012 04:29 AM, David Holmes wrote:
>> Testing:
>> JPRT
>
> JPRT will barely test anything related to argument processing.

Ok, thank you for this information. Can you recommend a way to test this 
change?

Thanks,
Erik

> David
> -----
>
>>
>> Also, running 'java -XX:+UseNUMA -XX:+PrintFlagsFinal -version' on a
>> system that does not support NUMA shows that 'UseNUMAInterleaving' and
>> 'MinHeapDeltaBytes' now have correct values.
>>
>> Thanks,
>> Erik