[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8283346: Optimize observable ArrayList creation in FXCollections [v4]
From:       Nir Lisker <nlisker () openjdk ! org>
Date:       2022-06-30 19:39:58
Message-ID: crLRV9BAy2J78-oOLzgAgYsTcfCKkWWrBfnjXE6S7DA=.d1a39e6f-9d1d-43f2-a8f8-19b2fd2844da () github ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]