On Tue, 28 Jun 2022 15:29:41 GMT, Marius Hanl <mh...@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
