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

[Stefan Johansson]

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

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.