Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Mon, 18 Dec 2023 22:48:18 GMT, Kim Barrett wrote: >> @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. @kimbarrett @dholmes-ora I just published this: https://github.com/openjdk/jdk/pull/17160 It removes the C-Heap capability from `GrowableArray`, and replaces usages with `GrowableArrayCHeap`. Bonus: we can now check that all element types of `GrowableArray` should be trivially destructible (that way it is always ok to abandon elements on the array, when the arena or ResourceMark go out of scope). - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1864429000
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Fri, 1 Dec 2023 07:56:04 GMT, Emanuel Peter 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 also completely refactored and improved ... Filed: JDK-8322476: Remove GrowableArray C-Heap version, replace usages with GrowableArrayCHeap Will work on that first, and then come back here later. - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1863061669
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Mon, 18 Dec 2023 22:48:18 GMT, Kim Barrett wrote: >> @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. @kimbarrett @dholmes-ora I will try to split out the GrowableArray cheap -> GrowableArrayCHeap changes. And thanks for the feedback you already gave, Kim! - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1862209037
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Fri, 1 Dec 2023 07:56:04 GMT, Emanuel Peter 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 also completely refactored and improved ... Splitting out part 3 would have been preferable IMO. The CHeap changes are unrelated to the capacity issue and should have their own JBS issue. - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1862149245
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Fri, 1 Dec 2023 07:56:04 GMT, Emanuel Peter 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 also completely refactored and improved ...
That's it for today. I'll continue looking at this tomorrow.
src/hotspot/share/utilities/bitMap.hpp line 191:
> 189: verify_size(size_in_bits);
> 190: }
> 191: ~BitMap() {}
This change is incorrect. This destructor is intentionally declared protected,
to prevent slicing
through it. It would be reasonable to change it to have a `= default`
definition though, rather
than the empty body definition it currently has.
Note that BitMap copying has the same shallow-copying problems as GrowableArray.
src/hotspot/share/utilities/growableArray.hpp line 87:
> 85: }
> 86:
> 87: ~GrowableArrayBase() {}
Another incorrect removal of an intentionally protected destructor.
src/hotspot/share/utilities/growableArray.hpp line 124:
> 122: GrowableArrayBase(capacity, initial_len), _data(data) {}
> 123:
> 124: ~GrowableArrayView() {}
Another incorrect removal of an intentionally protected destructor.
src/hotspot/share/utilities/growableArray.hpp line 294:
> 292: void remove_range(int start, int end) {
> 293: assert(0 <= start, "illegal start index %d", start);
> 294: assert(start < end && end <= _len, "erase called with invalid range
> (%d, %d) for length %d", start, end, _len);
pre-existing: I think start == end should be permitted. There's no reason to
forbid an empty range,
and there are algorithms that are simpler if empty ranges are permitted.
src/hotspot/share/utilities/growableArray.hpp line 319:
> 317: ::new ((void*)&this->_data[index]) E(_data[_len]);
> 318: // Destruct last element
> 319: this->_data[_len].~E();
Must not do the copy/destruct if index designated the last element.
src/hotspot/share/utilities/growableArray.hpp line 327:
> 325: // sort by fixed-stride sub arrays:
> 326: void sort(int f(E*, E*), int stride) {
> 327: qsort(_data, length() / stride, sizeof(E) * stride, (_sort_Fn)f);
pre-existing: Use of qsort presumes E is trivially copyable/assignable. Use
QuickSort::sort instead.
src/hotspot/share/utilities/growableArray.hpp line 398:
> 396: }
> 397:
> 398: ~GrowableArrayWithAllocator() {}
Another incorrect removal of an intentionally protected destructor.
src/hotspot/share/utilities/growableArray.hpp line 414:
> 412: // Assignment would be wrong, as it assumes the destination
> 413:
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Mon, 18 Dec 2023 07:49:04 GMT, David Holmes 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
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Mon, 18 Dec 2023 07:49:04 GMT, David Holmes 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
Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
On Fri, 1 Dec 2023 07:56:04 GMT, Emanuel Peter 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 also completely refactored and improved ... @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. - PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1859711325
RFR: 8319115: GrowableArray: Do not initialize up to capacity
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 also completely refactored and improved the tests for `GrowableArray(CHeap)`: https://github.com/openjdk/jdk/blob/e5eb36010355b444a719da6bdcd8c5de3145b961/test/hotspot/gtest/utilities/test_growableArray.cpp#L29-L60 The main improvement is that now **all** `GrowableArray` methods are tested, and that we test it with many different element types (including such without default-constructor or copy-assign-constructor). And we also check that the number of live elements is correct, which we can compute as `live = constructred - destructed`. This is especially valuable because I refactored the use of constructors/destructors heavily, to do the change from initializing up to `length` instead of `capacity`. **Note on move-semantics** Since move semantics is currently not allowed by the style guide, we have to "simulate" a move by placement new with copy-constructor, and then destruct the old element. See this example when we need to grow an array, and move the elements from the old data to the new data: https://github.com/openjdk/jdk/blob/e5eb36010355b444a719da6bdcd8c5de3145b961/src/hotspot/share/utilities/growableArray.hpp#L530-L563 Of course this is nothing new with my change here. I just want to record that we are doing it this way, and in fact have to do so without any move-semantics. The problem with this: If you use nested `GrowableArray>`, then the inner arrays get copy-constructed around when we re-allocate the outer array. We now have two choices for how `GrowableArray` could copy (currently it is a shallow-copy): - shallow-copy: works well for reallocating outer arrays, since the inner array is now just shallow-copied to the new data, and the destructor for the old inner arrays does nothing. Shallow-copy of course would not work for `GrowableArrayCHeap`, since it would deallocate the data of the old inner arrays, and the new inner array would still have a pointer to that deallocated data (bad!). But shallow-copy is generally dangerous, since the copy-constructor may be used in non-obvious cases: ResourceMark rm; GrowableArray> outer; outer.at_grow(100); // default argument calls default constructor, and (shallow) copy-constructs it so all elements outer.at(0).at_put_grow(0, 42); outer.at(1).at_put_grow(0, 666); // inner array at position 1 has reference to same data as inner array 0 ASSERT_EQ(outer.at(0).at(0), 42); // fails, we see 666 instead of 42 ASSERT_EQ(outer.at(1).at(0), 666); - deep-copy: This ensures correctness, we n
