8319115: GrowableArray: Do not initialize up to capacity #16918

eme64 · 2023-12-01T07:56:04Z

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):

jdk/test/hotspot/gtest/utilities/test_growableArray.cpp

Lines 29 to 60 in e5eb360

    
           // We have a list of each: 
        
           //  - ModifyClosure 
        
           //  - TestClosure 
        
           //  - AllocatorClosure 
        
           //  - AllocatorArgs 
        
           // 
        
           // For each AllocationClosure and AllocatorArgs, we call dispatch 
        
           // with each of the ModifyClosuresi and each TestClosures. The 
        
           // allocation cosure allocates its array (with initial size and 
        
           // capacity specified by the AllocatorArgs), and then passes itself 
        
           // into the ModifyClosure which does some first modificaions and 
        
           // subsequently into the TestClosure, which runs the test. 
        
           // 
        
           // For one test and allocator we do: 
        
           //   test.reset() 
        
           //   allocator.dispatch(modification, test); 
        
           //   -> allocate GrowableArray 
        
           //      modification.do_modify(allocator) 
        
           //      test.do_test(allocator) 
        
           //      -> call read / write ops on allocator which forwards 
        
           //         that to the allocated GrowableArray 
        
           //      de-allocate GrowableAray 
        
           //   test.finish() 
        
           // 
        
           // We test the arrays with different element types. Hence, the test 
        
           // is heavily templated. The idea is to ensure GrowableArray works 
        
           // for basic types, pointers, but also classes: for example with or 
        
           // without a default-constructor, or copy-assign operator. Of course 
        
           // if the copy-assign operator is deleted, then we cannot use certain 
        
           // write operations (such as at_put). With a special CtorDtor type we 
        
           // verify that constructor and destructor counts are as expected, 
        
           // for example we can verify the expected number of "live" elements.

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:

jdk/src/hotspot/share/utilities/growableArray.hpp

Lines 530 to 563 in e5eb360

    
           template <typename E, typename Derived> 
        
           void GrowableArrayWithAllocator<E, Derived>::expand_to(int new_capacity) { 
        
             int old_capacity = this->_capacity; 
        
             assert(new_capacity > old_capacity, 
        
                    "expected growth but %d <= %d", new_capacity, old_capacity); 
        
             // Allocate the new data with new capacity 
        
             this->_capacity = new_capacity; 
        
             E* new_data = static_cast<Derived*>(this)->allocate(); 
        
             // Copy-construct old->new (using placement new) 
        
             int i = 0; 
        
             for (     ; i < this->_len; i++) { 
        
               ::new ((void*)&new_data[i]) E(this->_data[i]); 
        
             } 
        
             // Leave rest of space up to "new_capacity" uninitialized, 
        
             // no construction 
        
             // Remove the old elements, calling the destructor 
        
             // (on initialized elements only) 
        
             for (i = 0; i < this->_len; i++) { 
        
               this->_data[i].~E(); 
        
             } 
        
             // Now that we have destructed all elements on the old 
        
             // data, we can deallocate it. 
        
             if (this->_data != nullptr) { 
        
               static_cast<Derived*>(this)->deallocate(this->_data); 
        
             } 
        
             // swap in the new data 
        
             this->_data = new_data; 
        
           }

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<GrowableArray<E>>, 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<GrowableArray<int>> 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 never have two arrays with the same underlying data. But that also means that when we re-allocate an outer array, we now (deep) copy-construct all new elements from the old elements. And that seems quite wasteful, both for the memory and the time needed to deep-copy everything over.

Neither of these options is good. This is exactly why the move-semantics were introduced in C++11. We should therefore discuss the introduction of move-semantics, and weigh it against the additional complexity that it introduces.

Testing:

tier1-3 and stress testing. Running.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Integration blocker

⚠️ Too few reviewers with at least role reviewer found (have 0, need at least 1) (failed with updated jcheck configuration in pull request)

Issue

JDK-8319115: GrowableArray: Do not initialize up to capacity (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16918/head:pull/16918
$ git checkout pull/16918

Update a local copy of the PR:
$ git checkout pull/16918
$ git pull https://git.openjdk.org/jdk.git pull/16918/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16918

View PR using the GUI difftool:
$ git pr show -t 16918

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16918.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-12-01T07:57:47Z

👋 Welcome back epeter! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-12-01T07:59:52Z

@eme64 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8319115
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-12-01T08:00:09Z

@eme64 The following labels will be automatically applied to this pull request:

graal
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

…expected

…erator deleted

mlbridge · 2023-12-18T07:20:05Z

Webrevs

00: Full (1ed037fd)

dholmes-ora · 2023-12-18T07:49:04Z

@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.

eme64 · 2023-12-18T09:11:59Z

@dholmes-ora
These are the "parts":

initialize up to capacity vs length
update the test to verify this (complete refactoring)
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 ;)

kimbarrett · 2023-12-18T22:48:18Z

@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

That's it for today. I'll continue looking at this tomorrow.

kimbarrett · 2023-12-18T22:44:18Z

src/hotspot/share/utilities/bitMap.hpp

  BitMap(bm_word_t* map, idx_t size_in_bits) : _map(map), _size(size_in_bits) {
    verify_size(size_in_bits);
  }
-  ~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.

kimbarrett · 2023-12-18T22:53:03Z