On Thu, 6 Nov 2025 20:08:31 GMT, jengebr <[email protected]> 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
