RFR: 8371164: ArrayList.addAll() optimizations [v5]

[Stuart Marks]

On Thu, 13 Nov 2025 15:05:28 GMT, jengebr <duke at openjdk.org> wrote:

>> test/jdk/java/util/Collection/MOAT.java line 905:
>> 
>>> 903:         arraySource.add(99);
>>> 904:         check(c.addAll(arraySource));
>>> 905:         equal(new ArrayList<Integer>(c), arraySource);
>> 
>> Here and at line 913 it seems a bit odd to copy `c` into a new ArrayList to compare equality. I think it's trying to assert that `c` contains only the expected elements. Unfortunately `c` can be any collection, not just a List, and to use List equality it needs to be copied into an actual List. Doubly unfortunately, the new List will capture the encounter order of whatever collection `c` is, which might not be well-defined -- for example if `c` is a HashSet. So I don't think this is the right assertion. Probably a size check and a containsAll() check, as is done in the bottom case, is sufficient.
>
> I'm curious, why not .equals() when we know the input is a List?  That is a stricter assertion because it ensures ordering remains unchanged.
> 
> Happy to make the change, though.

The testCollection1() method gets called with a bunch of different collections, including a HashSet, which arrives at this code in the `c` parameter. When `c` is copied into a new ArrayList, it gets whatever order is presented by `c`, which is unspecified when `c` is a HashSet. The resulting order might differ from the order in the source list, and equals() over Lists compares order, so it might result in a spurious failure.

I note that creating a HashSet of capacity 8 and adding 42 and then 99 results in [42, 99] but doing the same with a HashSet of capacity 16 results in [99, 42]. The test might actually pass, but if it does, it seems like it's pretty brittle.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2525367497