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

[Stefan Johansson]

Looks good,
Stefan

> 29 okt. 2019 kl. 21:44 skrev 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.