RFR: 8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap
Emanuel Peter
epeter at openjdk.org
Thu Dec 21 06:45:48 UTC 2023
On Wed, 20 Dec 2023 21:11:09 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> [JDK-8247755](https://bugs.openjdk.org/browse/JDK-8247755) introduced the `GrowableArrayCHeap`. This duplicates the current C-Heap allocation capability in `GrowableArray`. I now remove that from `GrowableArray` and move all usages to `GrowableArrayCHeap`.
>>
>> This has a few advantages:
>> - Clear separation between arena (and resource area) allocating array and C-heap allocating array.
>> - We can prevent assigning / copying between arrays of different allocation strategies already at compile time, and not only with asserts at runtime.
>> - We should not have multiple implementations of the same thing (C-Heap backed array).
>> - `GrowableArrayCHeap` is NONCOPYABLE. This is a nice restriction, we now know that C-Heap backed arrays do not get copied unknowingly.
>>
>> **Bonus**
>> We can now restrict `GrowableArray` element type `E` to be `std::is_trivially_destructible<E>::value == true`. The idea is that arena / resource allocated arrays get abandoned, often without being even cleared. Hence, the elements in the array are never destructed. But if we only use elements that are trivially destructible, then it makes no difference if the destructors are ever called, or the elements simply abandoned.
>>
>> For `GrowableArrayCHeap`, we expect that the user eventually calls the destructor for the array, which in turn calls the destructors of the remaining elements. Hence, it is up to the user to ensure the cleanup. And so we can allow non-trivial destructors.
>>
>> **Testing**
>> Tier1-3 + stress testing: pending
>
> 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.
@kimbarrett Thanks for looking at the PR!
I see you address a lot of "pre-existing" issues. And you would like GrowableArrayCHeap not have the MEMFLAGS in the template argument but maybe as a constructor argument instead. Or maybe a GACH version that only allocates once, though I guess that would limit what kinds of methods you could call on it...
Can we address these issues as separate RFE's?
-------------
PR Comment: https://git.openjdk.org/jdk/pull/17160#issuecomment-1865639695
More information about the serviceability-dev
mailing list