RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap

Emanuel Peter epeter at openjdk.org
Fri Dec 22 13:33:39 UTC 2023


On Fri, 22 Dec 2023 13:22:19 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> pre-existing: There are a lot of non-static class data members that are pointers to
>> GrowableArray that seem like they would be better as direct, e.g. non-pointers. 
>> 
>> pre-existing: There are a lot of iterations over GrowableArray's that would be
>> simplified by using range-based-for.
>> 
>> I'm not a fan of the additional clutter in APIs that the static memory types add.
>> If we had a variant of GrowableArrayCHeap that was not itself dynamically allocatable
>> and took a memory type to use internally as a constructor argument, then I think a
>> lot of that clutter could be eliminated.  It could be used for ordinary data members
>> that are direct GAs rather than pointers to GAs.  I think there is a way to do something
>> similar for static data members that are pointers that are dynamically allocated later,
>> though that probably requires more work.
>> 
>> I've not yet reviewed the changes to growableArray.[ch]pp yet, nor the test changes.
>> But I've run out of time and energy for this for today.
>
>> I'm not a fan of the additional clutter in APIs that the static memory types add. If we had a variant of GrowableArrayCHeap that was not itself dynamically allocatable and took a memory type to use internally as a constructor argument, then I think a lot of that clutter could be eliminated. It could be used for ordinary data members that are direct GAs rather than pointers to GAs. I think there is a way to do something similar for static data members that are pointers that are dynamically allocated later, though that probably requires more work.
> 
> FWIW, I added the GrowableArrayCHeap and the static memory type. I did that because there was a perceived need to minimize the memory usage, because we were going to use an extreme amount of these arrays for one of our subsystems in ZGC. It later turned out that we really didn't need to squeeze out the last bit of memory for that use-case. I would really like to get rid of the the static memory type from GrowableArrayCHeap, and just add it as an instance member.

@stefank Maybe it would then make sense to do that before this change here? Because we will really have to touch all these changes here again for that.

@kimbarrett @stefank @jdksjolen 
Or alternatively, we just say that we want to keep the C-Heap functionality inside `GrowableArray`, and simply remove `GrowableArrayCHeap`? Though I honestly would prefer a world where every allocation strategy has its own `GrowableArray*` variant, so that it is clear statically that they cannot be assigned to each other. So my preference would even be to have these:

CHeapGrowableArray
ArenaGrowableArray
ResourceAreaGrowableArray

What do you think?

And how important is it that we do not use `virtual` with `GrowableArray`? Because the implementation and testing is much nastier without it. Is the V-table still such a overhead that we need to avoid it?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17160#issuecomment-1867691744


More information about the serviceability-dev mailing list