RFR: 8213352: Separate BufferNode allocation from PtrQueueSet
stefan.johansson at oracle.com
Thu Nov 15 16:56:41 UTC 2018
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:
>> 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
>>> of free-lists by having one PtrQueueSet delegate to another; now
>>> free-list sharing is provided by using the same allocator object.
I think this is a really good change and generally it looks good. Just a
148 for (size_t i = 0; i < remove; ++i, prev = tail, tail =
149 assert(tail != NULL, "free list size is wrong");
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,
>>> 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.
More information about the hotspot-gc-dev