RFR: 8283346: Optimize observable ArrayList creation in FXCollections [v4]

[Nir Lisker]

On Tue, 28 Jun 2022 15:29:41 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> This simple PR optimizes the observable `ArrayList` creation by using the ArrayList constructor/array size so that the underlying array will be initialized at the correct size which will speed up the creation as the array does not need to grow as a result of the `addAll` call.
>> 
>> I also added tests which will succeed before and after to verify that nothing got broken by this change.
>> Also I made a benchmark test. Results:
>> 
>> | Benchmark | Mode| Cnt | Score | Error | Units
>> | ------------- | ------------- | ------------- | ------------- | ------------- | ------------- |
>> | ListBenchmark OLD  | thrpt | 25 | 722,842 | ± 26,93 | ops/s
>> | ListBenchmark NEW | thrpt  | 25 | 29262,274 | ± 2088,712 | ops/s
>> 
>> Edit: I also made a synthetic benchmark by measuring the same code below 100 times with `System.nanoTime`.
>> ListBenchmark OLD (avg): 21-23ms
>> ListBenchmark NEW (avg): 2 ms
>> 
>> <details><summary>Benchmark code</summary>
>> 
>> 
>> import javafx.collections.FXCollections;
>> import javafx.collections.ObservableList;
>> import org.openjdk.jmh.annotations.Benchmark;
>> import org.openjdk.jmh.annotations.Scope;
>> import org.openjdk.jmh.annotations.Setup;
>> import org.openjdk.jmh.annotations.State;
>> import org.openjdk.jmh.annotations.TearDown;
>> 
>> import java.util.ArrayList;
>> import java.util.List;
>> 
>> @State(Scope.Benchmark)
>> public class ListBenchmark {
>> 
>>     List<String> strings;
>> 
>>     @Setup
>>     public void setup() {
>>         strings = new ArrayList<>();
>>         for(int i = 0; i< 100000;i++) {
>>             strings.add("abc: " + i);
>>         }
>>     }
>> 
>>     @TearDown
>>     public void tearDown() {
>>         strings = null;
>>     }
>> 
>>     @Benchmark
>>     public ObservableList<String> init() {
>>         return FXCollections.observableArrayList(strings);
>>     }
>> }
>> 
>> 
>> </details>
>
> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8283346: add items directly to the backing list to save a change build caused by adding items to the observable list

Looks good. I left a few small comments.

I ran some benchmarks and I can reproduce the performance improvement both in the array (varargs) and the collection variants of the method.

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java line 351:

> 349:     public static <E> ObservableList<E> observableArrayList(E... items) {
> 350:         ArrayList<E> backingList = new ArrayList<>(items.length);
> 351:         backingList.addAll(Arrays.asList(items));

Unless I'm missing something again, this can be written as
`ArrayList<E> backingList = new ArrayList<>(Arrays.asList(items));`

I created benchmarks based on yours, and there is no significant difference between these two (their scores are within the error ranges of each other).

I don't mind leaving it as is if you think it's clearer, I personally prefer the 1 line variant.

modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java line 47:

> 45:     @Test
> 46:     public void testCreateObservableArrayListFromArray() {
> 47:         ObservableList<String> observableList = FXCollections.observableArrayList("1", "2", "3");

I would add a `null` element (like `"1", "2", null`) to test that the method accepts `null`s, especially after I fell for it myself.

modules/javafx.base/src/test/java/test/javafx/collections/FXCollectionsTest.java line 57:

> 55:     @Test
> 56:     public void testCreateObservableArrayListFromCollection() {
> 57:         List<String> list = List.of("1", "2", "3");

Same here with `null`, only now `Arrays.asList` will be needed instead.

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

PR: https://git.openjdk.org/jfx/pull/758