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

Erik Helin erik.helin at oracle.com
Mon Nov 19 13:19:16 PST 2012

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

More information about the hotspot-dev mailing list