RFR: 8213352: Separate BufferNode allocation from PtrQueueSet

[Stefan Johansson]

Hi Kim,

On 2018-11-08 00:55, Kim Barrett wrote:
>> On Nov 7, 2018, at 6:22 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Hi,
>>
>> On Sun, 2018-11-04 at 23:51 -0500, Kim Barrett wrote:
>>> Please review this change to PtrQueue buffer allocation, moving the
>>> implementation of free-list based allocation from PtrQueueSet to a
>>> separate BufferNode::Allocator object.  The old code supported
>>> sharing
>>> of free-lists by having one PtrQueueSet delegate to another; now
>>> free-list sharing is provided by using the same allocator object.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8213352
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8213352/open.00/
I think this is a really good change and generally it looks good. Just a 
small comments:
src/hotspot/share/gc/shared/ptrQueue.cpp
  148       for (size_t i = 0; i < remove; ++i, prev = tail, tail = 
tail->next()) {
  149         assert(tail != NULL, "free list size is wrong");
  150       }

I would prefer 'prev = tail, tail = tail->next()' to be in the body of 
the for loop to make it easier to read.
---

No need to see a new webrev for this and if you have a strong opinion 
against it go ahead and push anyways.

Thanks for doing this,
Stefan

>>>
>>> Testing:
>>> mach5 tier1-5
>>> Added gtest for new allocator class.
>>>
>>
>>   - copyright for the gtest should be 2018 only
> 
> Copy-paste from an existing test. Thanks for spotting it.
> 
>>   - out of curiosity, not an issue in the code, but why the need for
>> Atomic::load() in BufferNode::Allocator::free_count? It's okay, but I
>> do not see what Atomic::load provides you in this case compared to a
>> normal load. Is this the new preferred style to load from a volatile
>> variable of primitive type?
> 
> Maybe I shouldn’t do that, or maybe I should be proselytizing it.
> I’m finding the use of Atomic::(load|store) helpful by being explicit about
> relaxed loads/stores, rather than just “knowing" that’s what’s happening
> because the variable is declared volatile.  I’ve just recently started doing
> this sometimes in code I’m working on; this might be the first RFR with
> any of that.  There are currently only 31 other uses of those functions.
> 
>> Don't need to see the new webrev for the copyright date change.
> 
> Thanks.
>