On Sat, 8 Nov 2025 00:39:29 GMT, Stuart Marks <[email protected]> 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
