RFR: 8319117: GrowableArray: Allow for custom initializer instead of copy constructor [v8]

[Kim Barrett]

On Wed, 1 Nov 2023 10:37:21 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi, 
>> 
>> When using at_put and at_put_grow you can provide a value which will be supplied to the constructor of each element. In other words, you can intialize each element through a copy constructor.
>> 
>> I suggest that we also provide a function equivalent where the function is provided a pointer to the memory to be initialized. This can be used for `NONCOPYABLE` classes, for example.
>> 
>> This is implemented using a SFINAE pattern because `nullptr` introduces ambiguity if you use static overload.
>> 
>> Currently running tier1-tier4.
>
> Johan Sjölen has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Call dtr and global placement new
>  - Do not add this

src/hotspot/share/c1/c1_GraphBuilder.cpp line 181:

> 179:     block->init_stores_to_locals(method()->max_locals());
> 180:     _bci2block->at_put(cur_bci, block);
> 181:     _bci2block_successors.at_put_grow(cur_bci, BlockList(), BlockList());

I don't understand the purpose of this change.  BlockList is copyable and assignable, so the default filler
should be adequate.  What am I missing?

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

> 407:     assert(0 <= i, "negative index %d", i);
> 408:     if (i >= this->_len) {
> 409:       if (i >= this->_capacity) grow(i);

Unrelated to this change, but I think there is some inconsistency around the use of `grow()`.  Some callers
pass the minimum needed index (like here), some pass the minimum needed length (so 1 + min needed index).

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

> 412:         ::new (&this->_data[j]) E(args...);
> 413:       }
> 414:       this->_len = i + 1;

`grow()` will fill the range `[0,_len)` with copies of the current array contents, and `[_len,_capacity)` with
default constructed values.  Then we come here and replace the contents from `[_len,_capacity)`.  That
seems wasteful.  Maybe the args need to be passed through to `grow` and so to `expand_to`?

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

> 418: 
> 419:   template<typename... Args>
> 420:   void at_put_grow(int i, const E& elem, const Args&... args) {

I don't understand the point of this change, as the old code defaulted fill object seems like it should be fine
for at_put_grow, which requires the element type to be copyable/assignable anyway to store the new element
value.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16409#discussion_r1378829720
PR Review Comment: https://git.openjdk.org/jdk/pull/16409#discussion_r1378840762
PR Review Comment: https://git.openjdk.org/jdk/pull/16409#discussion_r1378845780
PR Review Comment: https://git.openjdk.org/jdk/pull/16409#discussion_r1378831528