On Fri, 22 Dec 2023 13:22:19 GMT, Stefan Karlsson <stef...@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
