On Mon, 18 Dec 2023 07:49:04 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Before this patch, we always initialized the GrowableArray up to its >> `capacity`, and not just up to `length`. This is problematic for a few >> reasons: >> >> - It is not expected. `std::vector` also only initializes the elements up to >> its size, and not to capacity. >> - It requires a default-constructor for the element type. And the >> default-constructor is then used to fill in the elements between length and >> capacity. If the elements do any allocation themselves, then this is a waste >> of resources. >> - The implementation also required the copy-assignment-operator for the >> element type. This is a lesser restriction. But the copy-assignment-operator >> was used in cases like `append` (where placement copy-construct would be >> expected), and not just in true assignment kinds of cases like `at_put`. >> >> For this reason, I reworked a lot of the methods to ensure that only the >> "slots" up to `length` are ever initialized, and the space between `length` >> and `capacity` is always garbage. >> >> ----- >> >> Also, before this patch, one can CHeap allocate both with `GrowableArray` >> and `GrowableArrayCHeap`. This is unnecessary. It required more complex >> verification in `GrowableArray` to deal with all cases. And >> `GrowableArrayCHeap` is already explicitly a smaller object, and should >> hence be preferred. Hence I changed all CHeap allocating cases of >> `GrowableArray` to `GrowableArrayCHeap`. This also allows for a clear >> separation: >> - `GrowableArray` only deals with arena / resource area allocation. These >> are arrays that are regularly abandoned at the end of their use, rather than >> deleted or even cleared. >> - `GrowableArrayCHeap` only deals with CHeap allocated memory. We expect >> that the destructor for it is called eventually, either when it goes out of >> scope or when `delete` is explicitly called. We expect that the elements >> could be allocating resources internally, and hence rely on the destructors >> for the elements being called, which may free up those internally allocated >> resources. >> >> Therefore, we now only allow `GrowableArrayCHeap` to have element types with >> non-trivial destructors, but `GrowableArray` checks that element types do >> not have non-trivial destructors (since it is common practice to just >> abandon arena / resource area allocated arrays, rather than calling the >> destructor or clearing the array, which also destructs all elements). This >> more clearly separates the two worlds: clean-up your own mess (CHeap) vs >> abandon your mess (arena / resource area). >> >> ----- >> >> I al... > > @eme64 Is it feasible to split this up to solve each of the problems you > identify in stages? There is also overlap here with JDK-8319709 IIUC. Thanks. @dholmes-ora These are the "parts": 1. initialize up to capacity vs length 2. update the test to verify this (complete refactoring) 3. remove cheap use of GrowableArray -> use GrowableArrayCHeap instead The first 2 items are inseparable, I cannot make substantial changes to many GrowableArray methods without there even being tests for them. And the tests would not pass before the changes for item 1, since the tests also verify what elements of the array are initialized. So adding the tests first would not be very feasible. The 3rd item could maybe be split, and be done before the rest. Though it would also require lots of changes to the test, which then I would have to completely refactor with items 1+2 anyway. And the items are related conceptually, that is why I would felt ok pushing them together. It is all about when (item 1) and what kinds of (item 3) constructors / destructors are called for the elements of the arrays, and verifying that thoroughly (item 2). Hence: feasible probably, but lots of work overhead. Do you think it is worth it? I am aware of JDK-8319709, and in conversation with @jdksjolen - I basically took this item over for him ;) ------------- PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1859859939
