RFR (S/M): 8213827: NUMA heap allocation does not respect process membind/interleave settings [Was: Re: [PATCH] JDK NUMA Interleaving issue]

[Thomas Schatzl]

Hi all,

  I prepared new webrevs with the suggestions from Sangheon, and I
think the changes Amit did:

http://cr.openjdk.java.net/~tschatzl/8213827/webrev.3_to_4/ (diff)
http://cr.openjdk.java.net/~tschatzl/8213827/webrev.4/ (full)

There were additional fixes:
- crash at startup with -XX:+UseNUMA since numa_interleave_memory() is
called before NUMA support has been initialized due to some earlier
refactoring (the cause is wrong library initialization order at startup
imho, but is to be handled separately).
- also CamelCased the enum values.
- removed some too compliated code ("return (a == b) ? true : false;"
and one other).
- removed some more random spacing

This patch passes hs-tier1-5 without additional failures now. Some
manual testing showed that it seems to do the right thing too now.

@Amith: please check if the change still works on your applications and
still gives the expected performance improvements.

Thanks,
  Thomas

On Thu, 2019-01-10 at 16:48 +0530, amith pawar wrote:
> Hi Sangheon,
> 
> Thanks again. I have done the required changes and created webrev.
> Please use following link to download the same as gmail is not
> allowing to attach.
> 
> https://drive.google.com/open?id=1QzmW6LdmKbBNHp4-hlcIr9DKY7anUMBy
> 
> Thanks,
> Amit
> 
> On Thu, Jan 10, 2019 at 1:03 AM <sangheon.kim at oracle.com> wrote:
> > Hi Amith,
> > 
> > On 1/9/19 3:30 AM, amith pawar wrote:
> > > Hi Sangheon,
> > > 
> > > Thanks for reviewing and updated with suggested changes. please
> > > check.
> >  Thank you for addressing my comments.
> > But I can't see below comments addressed: 
> > > > - Looking at 'enum' at os.hpp, we use Camel style.
> >  I meant to change from 'Numa_allocation_policy' to
> > 'NumaAllocationPolicy'.
> > > > - As usual, copyright year updates. I know it was correct when
> > > > you posted. :)
> >  Looking at the latest source code, only os_linux.hpp needs a new
> > copyright year.
> > - * Copyright (c) 1999, 2018, Oracle and/or its affiliates. All
> > rights reserved.
> > + * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All
> > rights reserved.
> > 
> > Looking at the v5, 
> > +      ls.print("UseNUMA is enabled and invoked in '%s' mode."
> > +                " Heap will be configured using NUMA memory
> > nodes:", numa_mode);
> > There is one more space before " Heap.... ", please remove it.
> > 
> > I see the latest version that Thomas posted is v3, but your
> > attached version is v5. :)
> > 
> > In addition, it would be better to provide webrev instead of a
> > patch. ( http://openjdk.java.net/guide/codeReview.html )
> > 
> > Thanks,
> > Sangheon
> > 
> > > Thanks,
> > > Amit Pawar
> > > 
> > > On Wed, Jan 9, 2019 at 12:45 AM <sangheon.kim at oracle.com> wrote:
> > > > Hi Thomas,
> > > > 
> > > > On 12/13/18 2:33 AM, Thomas Schatzl wrote:
> > > > > Hi Amit,
> > > > > On Thu, 2018-12-13 at 15:11 +0530, amith pawar wrote:
> > > > > > Hi Thomas,
> > > > > > 
> > > > > > Please find the attached patch updated as per your
> > > > > > suggestion.
> > > > > > If everything OK then can you please commit this to repo ?
> > > > > 
> > > > >   looks good. We will need a second reviewer though, I am
> > > > > going to ask
> > > > > around.
> > > > > 
> > > > > Latest webrev:
> > > > > http://cr.openjdk.java.net/~tschatzl/8213827/webrev.3/
> > > >  Webrev.3 looks good to me.
> > > > 
> > > > I have some minor nits:
> > > > ----------------------------------------
> > > > src/hotspot/os/linux/os_linux.cpp
> > > > 5012       for (int node = 0; node < Linux::numa_max_node();
> > > > node++) {
> > > > - Looks like 'node <= Linux::numa_max_node()' is the right one
> > > > to print the latest node?
> > > > 
> > > > ----------------------------------------
> > > > src/hotspot/os/linux/os_linux.hpp
> > > >  271   enum Numa_allocation_policy{
> > > > - Looking at 'enum' at os.hpp, we use Camel style.
> > > > - There are missing space before '{'.
> > > > 
> > > > - As usual, copyright year updates. I know it was correct when
> > > > you posted. :)
> > > > 
> > > > Thanks,
> > > > Sangheon
> > > > 
> > > > 
> > > > > Thanks,
> > > > >   Thomas
> > > > > 
> > > > > 
> > > >  
> > > > 
> > > 
> > > 
> > > -- 
> > > With best regards,
> > > amit pawar
> >  
> 
>