RFR: 8355013: GrowableArray default constructor should not allocate [v2]

[Stefan Karlsson]

On Fri, 18 Apr 2025 08:24:55 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Hi,
>> 
>> This patch changes the default constructors of `GrowableArray` so that it does not allocate. This is helpful because sometimes we create a `GrowableArray` and append another into it immediately, or create a `GrowableArray` to merge the value from several branches. In these cases, the default allocation is not needed. This also aligns the behaviour with that of `std::vector`, which does not allocate for default construction.
>> 
>> Please take a look and leave your reviews, thanks a lot.
>
> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refine tests

Are you sure that none of the current usages of `GrowableArray()` relies on the initial capacity to be `2`?

I've added a couple of suggestions for layout changes so that we can keep the structure of the constructors consistent. I think it makes it easier to see what this particular constructor is doing and how it differs from the other.

Also, previously, the constructors were order so that the CHeap version directly followed the Resource version:


GrowableArray() // Resource allocated

GrowableArray(int) // Resource allocated
GrowableArray(int, MemTag) // CHeap allocated

GrowableArray(int, int, const E&) // Resource allocated
GrowableArray(int, int, const E&, MemTag) // CHeap allocated


It would be nice to retain that structure. That is, just move the new `GrowableArray(MemTag)` up one step. Or, alternatively, completely separate the two groups of constructors into to sections.

src/hotspot/share/utilities/growableArray.hpp line 759:

> 757:   GrowableArray() : GrowableArrayWithAllocator<E, GrowableArray>(nullptr, 0), _metadata() {
> 758:     init_checks();
> 759:   }

Suggestion:

  GrowableArray() :
      GrowableArrayWithAllocator<E, GrowableArray>(
          nullptr,
          0),
      _metadata() {
    init_checks();
  }

src/hotspot/share/utilities/growableArray.hpp line 774:

> 772:     init_checks();
> 773:   }
> 774: 

Suggestion:

  explicit GrowableArray(MemTag mem_tag) :
      GrowableArrayWithAllocator<E, GrowableArray>(
          nullptr,
          0),
      _metadata(mem_tag) {
    init_checks();
  }

src/hotspot/share/utilities/growableArray.hpp line 836:

> 834: 
> 835: public:
> 836:   GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {}

Suggestion:

  GrowableArrayCHeap() :
      GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(
          nullptr,
          0) {}

src/hotspot/share/utilities/growableArray.hpp line 836:

> 834: 
> 835: public:
> 836:   GrowableArrayCHeap() : GrowableArrayWithAllocator<E, GrowableArrayCHeap<E, MT>>(nullptr, 0) {}

This change isn't necessary since the allocator doesn't allocate if you pass in `0`. But maybe you want this change for clarity?

-------------

PR Review: https://git.openjdk.org/jdk/pull/24748#pullrequestreview-2783150550
PR Review Comment: https://git.openjdk.org/jdk/pull/24748#discussion_r2053530538
PR Review Comment: https://git.openjdk.org/jdk/pull/24748#discussion_r2053529848
PR Review Comment: https://git.openjdk.org/jdk/pull/24748#discussion_r2053531623
PR Review Comment: https://git.openjdk.org/jdk/pull/24748#discussion_r2053551472