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

[jengebr]

On Thu, 13 Nov 2025 00:27:10 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> jengebr has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Whitespace fixes
>
> test/jdk/java/util/Collection/MOAT.java line 890:
> 
>> 888:     }
>> 889: 
>> 890:     private static void testAddAll(Collection<Integer> c) {
> 
> Thanks for moving this test into MOAT. Overall it seems like a large reduction in test bulk, which simplifies a lot of things. Great!

Yes, thank you for pointing it out!  I overlooked its existence until you pointed me to it, I agree it's a a much better option.

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

> test/jdk/java/util/Collection/MOAT.java line 922:
> 
>> 920:         check(c.addAll(arraySource));
>> 921:         equal(c.size(), sizeBefore + arraySource.size());
>> 922:         check(c.containsAll(arraySource));
> 
> As I said in another comment, this (size check and containsAll check) looks like a better set of assertions than using List equality as in the earlier test cases.
> 
> I'm confused about the scope of cases being covered here. It seems like there are potentially three different axes of cases that potentially could be tested:
> 
> 1) source is ArrayList / other kind of List
> 2) source is empty / not empty
> 3) destination is empty / not empty
> 
> That would indicate having eight test cases. That's kind of at the outer limit for hand-coded cases. At this point or beyond, having some automated case generation would be preferable. And this is MOAT and not JUnit, so test generation would have to be done _ad hoc_. I could imagine doing it in about the same amount of code (say 30 or fewer lines). But if you're not up for doing this, it's probably sufficient for this change to test just the ArrayList and non-ArrayList sources, since that should be sufficient to test the code path you're changing.

> At this point or beyond, having some automated case generation would be preferable. And this is MOAT and not JUnit, so test generation would have to be done ad hoc

Thank you!  I will reduce the testing to the dimension #1.  Your suggestion of a JUnit-based equivalent is excellent, I'll work it in sometime in the future.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523821114
PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523814719
PR Review Comment: https://git.openjdk.org/jdk/pull/28116#discussion_r2523810534