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

Tue Nov 5 08:24:32 UTC 2019

Hi Sangheon,

Again, thanks for working through our comments. Some inline responses 
and one minor additional thing.

On 2019-11-05 07:22, sangheon.kim at oracle.com wrote:
> Hi Stefan,
> 
> On 11/4/19 8:20 AM, Stefan Johansson wrote:
>> Hi Sangheon,
>>
>> Thanks for addressing my comments, some comments inline and additional 
>> comments below.
>>
>> On 2019-11-02 07:08, sangheon.kim at oracle.com wrote:
>>>> ...
>>>> As we discussed offline, I would like to know the mismatches as 
>>>> well, I think the easiest approach would be to make the total count 
>>>> per node as well and that way we can see if there were any regions 
>>>> that didn't match. What do you think about printing the info like this:
>>>> [3,009s][trace][gc,heap,numa ] GC(6) NUMA region verification 
>>>> (actual/expected): 0: 1024/1024, 1: 270/1024, Unknown: 0
>>> Changed as you suggested to have per node but deleted unknown as 
>>> unknown is 'total - matched'.
>> I'm fine with skipping 'Unknown', but not all regions in (total - 
>> matched) must be 'Unknown' since there is the possibility of a real 
>> mismatch as well.
> Basically I agree with you that 'total - matched' remains some other 
> cases. i.e. 'mismatch' and 'unknown (mostly not yet touched)'. 
> Previously I just wanted to highlight the most interesting part which I 
> think is 'matched and total'.
> 
> New patch counts, matched / mismatched / total.
This is great, then we all get the info we like :)

> ...
>> Any reason for not including it at the end of GC as well? I guess that 
>> could be good to be able to diagnose if there was an increase 
>> imbalance due to the collection.
> Added after gc as well.
> I just wanted to avoid logging to much. But as you said, node 
> information after gc seems helpful too. 
> ...
>> I realize I didn't include that in my example patch, but did you have 
>> anything against changing the tags here to 'gc, heap, verify'?
> I'm with gc+heap+verify so update it.
Nice =)

> 
> Added 'mismatch' which doesn't count active_node_index == UnknownNodeIndex.
> So now we are printing 'matched / mismatched / total'. We are not 
> printing unknown but 'total = matched + mismatched + unknown', so we can 
> calculate it.
> 
> One may want to explicitly print 'unknown' additionally or print 
> 'unknown' instead of 'total'.
> Or we can split into 2 levels too.
> But I think matched / mismatched / total seems okay for now.
>
Agreed

>> ...
>> I think we should differ between the case where the call fails (return 
>> -1) and the id returned is negative. From reading the man_pages for 
>> 'move_pages()' (used by numa_move_pages()) it seems we can expect to 
>> either get -EFAULT (for normal pages) or -ENOENT (for large pages) 
>> when a region has not yet been used. I'm not completely sure how much 
>> logic we want to add to this method, so maybe creating a RFE to look 
>> at this in more detail later is good. But for this first version I 
>> think your proposal is good.
> I understand your comment to distinguish 'not yet faulted in (not yet 
> touched)' case which seems nice to have. But I don't have good usage for 
> it on top of my head. This may be used when record / print at 
> NodeIndexCheckClosure::do_heap_region().
> 
> Filed a RFE, https://bugs.openjdk.java.net/browse/JDK-8233535
>
Thanks, I'm not sure about the use case either so a new RFE to 
investigate is good.

>>
>> One thought I just got, with this implementation is more like 
>> 'Unused', rather then 'Unknown'. But such a notion change could also 
>> be looked at in the RFE.
> I'm not sure I fully agree / understand with you. :)
> 
> I think when we combine 'unused or unknown' with 'node index / id', 
> unknown seems better fit.
> 'unknown node index' which means the value is unknown for some 
> reason(mostly not yet faulted in) seems better than 'unused node index' .
> To me 'unused' seems better when combined with 'page or address'. e..g 
> unused page or unused address etc.
> 
> If you still think further notion change is necessary in separate CR, I 
> can file for you. :)
I agree with you that currently Unknown is better, but one possible use 
case if distinguish between failing the syscall and getting the expected 
status for not yet faulted pages would be that we could say that some 
pages are unused and some are unknown (but that would probably lead to 
most just being unused).

Well, I don't need another CR for this, just thinking out in the mail =)

> 
> 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.
---

Thanks,
Stefan

> 
> - All comments from Kim and Stefan.
> - Includes class / methods name changes which Kim suggested to me directly.
> 
> Testing: hs-tier 1 ~ 5, with/without UseNUMA.
> 
> Thanks,
> Sangheon
> 
> 
>> ---
>>
>> Thanks,
>> Stefan
>>
>>>
>>> Testing: hs-tier 1 ~ 5 with / without UseNUMA.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>> ---
>>>>
>>>> This is it for now. Thanks,
>>>> Stefan
>>>>
>>>>
>>>>> Testing: hs-tier 1 ~ 4 with/without UseNUMA. hs-tier5 is almost 
>>>>> finished without new failures.
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>
> 