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

[David Holmes]

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