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

[David Holmes]

Hi Erik,

This seems to be a potentially very disruptive change. Argument 
management is in general very fragile and easily broken, so major 
changes like this are difficult to validate because it can be the 
combinations of different args that cause the problem.

On 30/10/2012 12:29 AM, Erik Helin wrote:
> the webrev can be found at:
>
> http://cr.openjdk.java.net/~jwilhelm/7198334/webrev/
>
> Summary:
> The bug existed because the function 'Arguments::parse' changed the
> value of the flags 'UseNUMAInterleaving' and 'MinHeapDeltaBytes'
> depending on the value of the flag 'UseNUMA'.
>
> The problem was that the function 'os::init_2' checks if NUMA is
> supported on the system, and if it is not supported, sets 'UseNUMA' to
> 'false'.
>
> 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?

> The fix splits the function 'Arguments::parse' into three functions:
> - 'Arguments::parse' now only parses the arguments.
> - 'Arguments::adjust_before_os' adjusts the flags that must be adjusted
> *before* 'os::init_after_parsing_flags'.
> - 'Arguments::adjust_after_os' adjusts the flags that must be adjusted
> *after* 'os::init_after_parsing_flags'.
>
> Note that three functions are necessary since the flag 'UseLargePages'
> has to be adjusted *before* 'os::init_2' is called.

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.

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

> Testing:
> JPRT

JPRT will barely test anything related to argument processing.

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