On Wed, 20 Dec 2023 21:11:09 GMT, Kim Barrett <kbarr...@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
