RFR(M): 8220311: Implementation: NUMA-Aware Memory Allocation for G1, Survivor (2/3)

[sangheon.kim at oracle.com]

Hi Stefan,

Thanks for reviewing this.

On 10/29/19 1:13 PM, Stefan Johansson wrote:
> Hi Sangheon,
>
>> 25 okt. 2019 kl. 16:02 skrev sangheon.kim at oracle.com:
>>
>> Hi Stefan,
>>
>> On 10/23/19 1:47 AM, Stefan Johansson wrote:
>>> Hi Sangheon,
>>>
>>> On 2019-10-22 18:47, sangheon.kim at oracle.com wrote:
>>>> Hi Kim,
>>>>
>>>> On 10/22/19 12:19 AM, Kim Barrett wrote:
>>>>>> On Oct 22, 2019, at 1:52 AM, sangheon.kim at oracle.com wrote:
>>>>>> What do you think about below comment?
>>>>>>
>>>>>>     // Tries to allocate word_sz in the PLAB of the next "generation" after trying to
>>>>>>     // allocate into dest. Previous_plab_refill_failed indicates whether previous
>>>>>>     // PLAB refill for the original (source) object was failed.
>>>>> Drop “was”.  Otherwise looks good.
>>>> Done.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sangheki/8220311/webrev.3
>>>> http://cr.openjdk.java.net/~sangheki/8220311/webrev.3.inc
>>> Looks good in general, just one minor thing, no need for a new webrev though:
>>> src/hotspot/share/gc/g1/g1Allocator.cpp
>>> ---
>>> 144   for (uint nodex_index = 0; nodex_index < _num_alloc_regions; nodex_index++) {
>>>
>>> The name nodex_index has one too many x:es =) I would prefer node_index.
>> Ouch!
>> Fixed..
>>
>> In addition, Stefan, Thomas and I had some discussion about making PLAB-NUMA aware (only for survivor).
>> Stefan provided a patch with it and it is simple enough to include under this CR.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~sangheki/8220311/webrev.4
>> http://cr.openjdk.java.net/~sangheki/8220311/webrev.4.inc
> Looks good in general, just one comment.
>
> src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> ---
>    78   assert(_alloc_buffers[dest.type()] != NULL,
>    79          "Allocation buffer is NULL: %s", dest.get_type_str());
>
>    80   G1HeapRegionAttr::region_type_t type = dest.type();
>    81   return alloc_buffer(type, node_index);
>
> As I mentioned to you offline, I think it is a bit unfortunate that we can’t index our way to the correct PLAB in G1PLABAllocator::alloc_buffer(…) without the if-statement, but I agree that having multiple array slots pointing to the same PLAB isn’t a optimal either. So I think this is approach is good for now, but I have a very minor comment on the code snippet above. I would prefer if line 80 was skipped and the call on 81 just did return alloc_buffer(dest.type(), node_index).
Done.
It is leftover from testing code.

You and Thomas didn't ask for webrev, but here's the next one for the 
record. :)

Webrev:
http://cr.openjdk.java.net/~sangheki/8220311/webrev.5
http://cr.openjdk.java.net/~sangheki/8220311/webrev.5.inc

Testing: local build

Thanks,
Sangheon


> —
>
> I don’t need a new webrev for this.
>
> Thanks,
> Stefan
>
>
>> Testing: hs-tier 1 ~ 3, with/without UseNUMA
>>
>> Thanks,
>> Sangheon
>>
>>
>>> ---
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>>>     // Returns a non-NULL pointer if successful, and updates dest if required.
>>>>>>     // Also determines whether we should continue to try to allocate into the various
>>>>>>     // generations or just end trying to allocate.
>>>>>>     HeapWord* allocate_in_next_plab(G1HeapRegionAttr* dest,
>>>>>> ...
>>>>>>
>>>>>> Let me post the webrev when we decide. :)
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>>
>>>>>>> Looks good, other than that one comment issue.