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

Erik Helin erik.helin at oracle.com
Tue Nov 20 02:50:47 PST 2012

Thanks Bengt!

Erik

On 11/20/2012 11:32 AM, Bengt Rutisson wrote:

Erik, Looks good to me too. I can help you push this. Bengt

On 2012-11-20 09:44, 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::init2' in the function >>>> 'Threads::createvm', 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::init2 into an earlier function - os::init >>> perhaps? >> >> We could adjust UseNUMAInterleaving and MinHeapDeltaBytes in os::init2 >> 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::init2() > code could simply call Arguments::adjustgcargs() (or some such name). > > That said I don't think the factoring of the changes into > adjustparallelgcflagsafteros() 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 adjustafteros. 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::adjustafteros. >> >> I do not think that simply splitting Arguments::parse into two >> functions, Arguments::parse and Arguments::adjustbeforeos is a >> disruptive change if Arguments::adjustbeforeos 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 adjustbefore. 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::initbeforeparsingflags' >>>> and 'os::init2' to 'os::initafterparsingargs'. 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::initpreargs() and >>> os::initpostargs() ? 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