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

[Stuart Marks]

On Thu, 6 Nov 2025 20:08:31 GMT, jengebr <duke at openjdk.org> wrote:

>> jengebr has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Adding direct unit tests, minor revisions to optimizations
>
> 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://github
 .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 assertion checks over the actual result.

It might be easier to do write this as a JUnit test instead of as open code in an old-school style `main` test. Or, it could be done as some enhancements to the MOAT tests. This poses its own challenges, but fundamentally it isn't difficult; you just have to wallow in that style for a while. :-)

I didn't look too closely at the `testModCountIncrement()` test method, but I'll observe that both JUnit and MOAT have an "expect this statement to throw an exception" construct.

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

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