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 too would prefer that it be split up. It's very easy to miss important details in amongst all the mostly relatively simple renamings. That is, I think 3 should be separate from the other changes. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1861812983
