RFR(L): 8220312: Implementation: NUMA-Aware Memory Allocation for G1, Logging (3/3)

[sangheon.kim at oracle.com]

Hi all,

Many thanks for your through reviews, Kim, Stefan and Thomas!

You didn't request for the next webrev, but let me post for the record.

Webrev:
http://cr.openjdk.java.net/~sangheki/8220312/webrev.6
http://cr.openjdk.java.net/~sangheki/8220312/webrev.6.inc

Testing: local build

Thanks,
Sangheon


On 11/6/19 1:32 AM, Thomas Schatzl wrote:
> Hi,
>
> On 05.11.19 20:57, Kim Barrett wrote:
>>> On Nov 5, 2019, at 3:24 AM, Stefan Johansson 
>>> <stefan.johansson at oracle.com> wrote:
>>> On 2019-11-05 07:22, sangheon.kim at oracle.com wrote:
>>>> webrev:
>>>> http://cr.openjdk.java.net/~sangheki/8220312/webrev.5
>>>> http://cr.openjdk.java.net/~sangheki/8220312/webrev.5.inc
>>>
>>> This all looks good, but there is an else-if statement that you can 
>>> simplify a bit:
>>> src/hotspot/share/gc/g1/g1NUMA.cpp
>>> ---
>>> 297   if (preferred_node_index == active_node_index) {
>>> 298     _matched[preferred_node_index]++;
>>> 299   } else if (preferred_node_index != active_node_index &&
>>> 300              active_node_index != G1NUMA::UnknownNodeIndex) {
>>> 301     _mismatched[preferred_node_index]++;
>>> 302   }
>>>
>>> The first condition in the else-if statement will always be true (or 
>>> the if-branch will be taken) and can be removed.
>>
>> Looks good to me too, except for that same else-if.  I don’t need a 
>> new webrev for that.
>>
>>
>
> same.
>
> Plus please rename NodeIndexCheckClosure to G1NodeIndexCheckClosure (I 
> know that HeapRegion* does not have a G1 prefix either, but let's not 
> add to the issue).
>
> I believe this rename can be done without a re-review from me.
>
> Thanks,
>   Thomas