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

[jengebr]

On Sat, 8 Nov 2025 00:39:29 GMT, Stuart Marks <smarks at openjdk.org> wrote:

>> Further investigation into the duplication/de-duplication of the fastpath shows that I *cannot reproduce* the negative results - they are clearly logged, I simply can't generate new runs with the same data.  No idea why.
>> 
>> I've updated the code with the simplified branching, all comments are addressed.
>
> @jengebr Thanks for rechecking the benchmark. The ArrayList change looks much more sensible now, and I'm glad it performs to your expectations!
> 
> Regarding tests... I see you added a new test `SingletonSetToArray`. I took a quick look and it seems to me that these paths are already tested by the Collection [MOAT](https://github.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java) test. This test is somewhat difficult to follow, but it tries to test all the proper assertions over all the different collection implementations. For this case it creates a singleton set at [line 201](https://github.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java#L201), which calls [testCollection()](https://github.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java#L1243), which calls [testCollection1()](https://github.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java#L1248), which eventually calls [checkFunctionalInvariants()](https://github.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java#L793). And finally at [line 815](https://gith
 ub.com/openjdk/jdk/blob/jdk-26%2B23/test/jdk/java/util/Collection/MOAT.java#L815) it checks toArray() with arrays of various lengths. So it may be that `SingletonSetToArray` isn't necessary. I see it does a check with a singleton set containing null, but I'm not sure that's actually necessary.
> 
> In the changes to the `ArrayList/AddAll.java` there is a test method `testSingletonSetToArray()`. Is this necessary, especially in light of the above?
> 
> Also in `ArrayList/AddAll.java` there are test methods `testArrayListFastPath()` and `testArrayListSubclassUsesSlowPath()`. These don't actually test whether the fast path or the slow path is used (and indeed, doing so would be hard, but we might have a discussion about how that could be tested, or even whether it should be tested). Instead, they mainly test that the *results* are as expected when calls are made that go through the different code paths. Hm, it doesn't seem like MOAT tests addAll(); that seems like an oversight. Well, it seems like some simplifications that can be done. There are a few combinations of inputs:
> 
>  * source is ArrayList or ArrayList subclass
>  * source is empty or non-empty
>  * destination is empty or non-empty
>  * source contains nulls or not
> 
> (I'm not convinced we need separate cases for null in the contents.) In every case though there should be a unified set of asserti...

@stuart-marks thank you for the detailed feedback!  I've updated as follows:
1. Reverted ArrayList/AddAll, deleted the singleton test.
2. Added `testAddAll(Collection<Integer> c)` to MOAT
3. I implemented mostly as you described:
  1. I agree with "I'm not convinced we need separate cases for null in the contents." and did not implement them.
  1. I verified functionality of fast- and slow-paths, but did not attempt to verify which path executed.
  2. I stuck with MOAT-style.  JUnit has plenty of advantages, but this fits as a small change to the larger test.

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

PR Comment: https://git.openjdk.org/jdk/pull/28116#issuecomment-3512681722