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

[Erik Helin]

Thanks David!

Erik

On 11/20/2012 09:44 AM, David Holmes wrote:
> Thanks Erik, I have no further comments on this. :)
>
> You still need a second reviewer of course.
>
> Cheers,
> David
>
> On 20/11/2012 7:19 AM, Erik Helin wrote:
>> Hi David,
>>
>> first of all, sorry for my very late reply, I've had some serious
>> technical issues with my email account.
>>
>> This is a reply to the following email:
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2012-October/007128.html
>>
>>
>> Unfortunately, my email address has been changed and I can't therefore
>> reply correctly to the original email.
>>
>> I've uploaded a new, minimal webrev at:
>> http://cr.openjdk.java.net/~mgerdin/JDK-7198334.1/
>>
>> On 10/30/2012 6 PM, David Holmes wrote:
>>   >>  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.
>>
>> I agree, I've folded the update of the flags into the function
>> adjust_after_os.
>>
>> On 10/30/2012 6 PM, David Holmes wrote:
>>   >>  On 30/10/2012 12:29 AM, Erik Helin wrote:
>>   >>  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.
>>
>> I agree with you that my change did too much. If (I'm not saying that it
>> should) the code should be refactored, then it should be done in another
>> change, not the same as the one for the bugfix.
>>
>> On 10/30/2012 6 PM, David Holmes wrote:
>>   >>>>  On 29/10/2012 12:29 AM, Erik Helin 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.
>>   >>>
>>   >>>  On 10/30/2012 04:29 AM, David Holmes wrote:
>>   >>>  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.
>>   >>
>>   >>  On 30/10/2012 12:29 AM, Erik Helin wrote:
>>   >>  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!
>>
>> I think this is a very interesting discussion, and think we should
>> continue
>> it "off-line", but I no longer think that it should be a part of the
>> discussion for this change with the new minimal webrev (see top of
>> email).
>>
>> On 10/30/2012 6 PM, David Holmes wrote:
>>   >  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.
>>
>> I agree and I've updated the webrev accordingly, what do you think of
>> the new one?
>>
>> Thanks for taking your time and reviewing this change,
>> Erik
>>
>> On 10/30/2012 6 PM, David Holmes wrote:
>>   >  Cheers,
>>   >  David
>>   >
>>   >
>>   >>
>>   >>  On 30/10/2012 12:29 AM, Erik Helin wrote:
>>   >>
>>   >>>>  On 29/10/2012 12:29 AM, Erik Helin wrote:
>>   >>>>  Testing:
>>   >>>>  JPRT
>>   >>>
>>   >>>  On 10/30/2012 04:29 AM, David Holmes wrote:
>>   >>>  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
>>   >>>  -----
>>   >>>
>>   >>>>  On 29/10/2012 12:29 AM, Erik Helin wrote:
>>   >>>>  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