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

[Erik Helin]

Thanks Jesper!

Erik

On 11/20/2012 02:56 PM, Jesper Wilhelmsson wrote:
> Looks good!
> Ship it!
> /Jesper
>
> On 11/20/12 9: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
>