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

[Erik Helin]

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