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

[David Holmes]

Hi Erik,

On 31/10/2012 1:14 AM, Erik Helin wrote:
> Please see an updated webrev at:
>
> http://cr.openjdk.java.net/~mgerdin/JDK-7198334/

Okay - general restructure is much more palatable.

> I've responded inline.

As have I.

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

Ok, but in a similar style to what you have introduced, the os::init_2() 
code could simply call Arguments::adjust_gc_args() (or some such name).

That said I don't think the factoring of the changes into 
adjust_parallel_gc_flags_after_os() is warranted in the current 
proposal. It isn't being called from multiple places, nor from any 
Arguments client, so there's really no need to expand the Arguments API 
this way.

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

Fair enough.

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

I don't think it is completely clear what changes can be done in parse 
versus what have to be deferred to adjust_before. So in my view this 
split does not directly aid understandability. I do understand that as a 
newcomer to the codebase this changes aids your understanding of the code.

 From a pragmatic perspective such changes are highly subjective and 
they add "noise" to the reviewing process.

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

Restricting the movement to only the code that needs moving is good.

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

I am not a fan of "code speaking for itself" if the code "talks too 
much" - that is why languages have comments. Speaking belongs in 
comments. Code should be clear without being overly verbose, yet 
succinct without being cryptic. And code can certainly become outdated 
if that code is the name of a method. If the role of the method expands 
then it is better to adapt a comment than to revise an API to rename a 
method and update all callsites!

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

I do not support that. This is highly subjective of course, but your 
changes fall beyond where I draw my line. :)

If you did not do this renaming your webrev would be exceedingly short 
compared to the current webrev, and your changes would only touch on GC 
related bits of code. The renaming affects the OS API and that is 
general runtime code. So I will defer to Karen as the owner of the 
runtime code.

Cheers,
David


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