8283346: Optimize observable ArrayList creation in FXCollections #758

Maran23 · 2022-03-17T21:10:14Z

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

Benchmark code

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);
    }
}

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8283346: Optimize observable ArrayList creation in FXCollections

Reviewers

Michael Strauß (@mstr2 - Author)
Nir Lisker (@nlisker - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/758/head:pull/758
$ git checkout pull/758

Update a local copy of the PR:
$ git checkout pull/758
$ git pull https://git.openjdk.org/jfx pull/758/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 758

View PR using the GUI difftool:
$ git pr show -t 758

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/758.diff

bridgekeeper · 2022-03-17T21:11:07Z

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-03-17T21:16:04Z

Webrevs

yososs · 2022-03-18T21:28:33Z

This PR appears to have two internal behavior changes.

A. Initial collection size (PR owner's description)
B. By adding the item directly to the backingList
Avoiding doAdd () -- beginChange () -- nextAdd() --endChange ()

ModifiableObservableListBase.java

    @Override
    public void add(int index, E element) {
        doAdd(index, element);
        beginChange();
        nextAdd(index, index + 1);
        ++modCount;
        endChange();
    }

I don't know what the modCount property is used for, but is it okay if the modified code has different values?

ObservableListWrapper.java

     @Override
     protected void doAdd (int index, E element) {
         if (elementObserver! = null)
             elementObserver.attachListener (element);
         backingList.add (index, element);
     }

ObservableListWrapper.java

    public ObservableListWrapper(List<E> list) {
        backingList = list;
        elementObserver = null;
    }

We will need to make sure that the current unit tests cover B's changes.

mstr2 · 2022-03-19T00:28:49Z

The following pieces of code should be identical:

var list = FXCollections.observableArrayList();
list.addAll(source);

var list = FXCollections.observableList(new ArrayList<>(source));

Any observable difference in behavior would be unspecified. When it comes to modCount, that's an internal field of AbstractList.
FXCollections.observableArrayList() and FXCollections.observableList(...) are not the right place to test this.

Maran23 · 2022-03-19T10:02:38Z

I don't know what the modCount property is used for, but is it okay if the modified code has different values?

This is fine as the mod count is only there to detect concurrent modifications.
I recommend to read the modCount javadoc, it is very well written.
And as @mstr2 also wrote, it is also an internal field (for the above mentioned purpose).

kevinrushforth · 2022-03-19T16:51:33Z

/reviewers 2

openjdk · 2022-03-19T16:52:25Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java

…d caused by adding items to the observable list

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java

nlisker

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/test/java/test/javafx/collections/FXCollectionsTest.java

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java

Maran23 · 2022-07-01T07:56:32Z

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.

Thanks for the review and verifying the improvement.
I implemented your suggested changes.

openjdk · 2022-07-01T16:26:32Z

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8283346: Optimize observable ArrayList creation in FXCollections

Reviewed-by: mstrauss, nlisker, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 46 new commits pushed to the master branch:

1f62570: 8285253: Update boot JDK to 18.0.1
c759286: 8286774: Replace openjdk.java.net with openjdk.org
da5bd37: 8277756: DatePicker listener might not be added when using second constructor
864792d: 8284654: Modal behavior returns to wrong stage
83a46e0: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
f534850: 8088420: JavaFX WebView memory leak via EventListener
d677003: 8286256: Update libxml2 to 2.9.14
19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
81e1cc3: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
... and 36 more: https://git.openjdk.org/jfx/compare/eb7fa5dd1c0911bca15576060691d884d29895a1...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@nlisker, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Maran23 · 2022-07-02T12:18:34Z

/integrate

openjdk · 2022-07-02T12:19:07Z

@Maran23
Your change (at version 8fe4603) is now ready to be sponsored by a Committer.

kevinrushforth · 2022-07-02T12:36:28Z

/sponsor

openjdk · 2022-07-02T12:37:19Z

Going to push as commit b3eca1f.
Since your change was applied there have been 46 commits pushed to the master branch:

1f62570: 8285253: Update boot JDK to 18.0.1
c759286: 8286774: Replace openjdk.java.net with openjdk.org
da5bd37: 8277756: DatePicker listener might not be added when using second constructor
864792d: 8284654: Modal behavior returns to wrong stage
83a46e0: 8284665: First selected item of a TreeItem multiple selection gets removed if new items are constantly added to the TreeTableView
f534850: 8088420: JavaFX WebView memory leak via EventListener
d677003: 8286256: Update libxml2 to 2.9.14
19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)
81e1cc3: 8286552: TextFormatter: UpdateValue/UpdateText is called, when no ValueConverter is set
... and 36 more: https://git.openjdk.org/jfx/compare/eb7fa5dd1c0911bca15576060691d884d29895a1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-02T12:37:34Z

@kevinrushforth @Maran23 Pushed as commit b3eca1f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8283346: Optimized observable array list creation

30262a5

openjdk bot added the rfr Ready for review label Mar 17, 2022

8283346: Better test name

241962a

kevinrushforth requested review from kevinrushforth and arapte March 19, 2022 16:51

8283346: Improved test name

a92e9a0

mstr2 reviewed Jun 22, 2022

View reviewed changes

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java Outdated Show resolved Hide resolved

8283346: add items directly to the backing list to save a change buil…

3f28840

…d caused by adding items to the observable list

nlisker reviewed Jun 28, 2022

View reviewed changes

modules/javafx.base/src/main/java/javafx/collections/FXCollections.java Outdated Show resolved Hide resolved

mstr2 approved these changes Jun 28, 2022

View reviewed changes

nlisker reviewed Jun 30, 2022

View reviewed changes

Maran23 added 2 commits July 1, 2022 09:51

8283346: Refactoring: Using the ArrayList constructor directly

aa52c3b

8283346: Added null to the test data

28fbd34

8283346: Inline ArrayList creation

8fe4603

nlisker approved these changes Jul 1, 2022

View reviewed changes

mstr2 approved these changes Jul 1, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jul 1, 2022

kevinrushforth approved these changes Jul 2, 2022

View reviewed changes

openjdk bot added the sponsor Ready to sponsor label Jul 2, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 2, 2022

openjdk bot closed this Jul 2, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jul 2, 2022

8283346: Optimize observable ArrayList creation in FXCollections #758

8283346: Optimize observable ArrayList creation in FXCollections #758

Maran23 commented Mar 17, 2022 •

edited by openjdk bot

bridgekeeper bot commented Mar 17, 2022

mlbridge bot commented Mar 17, 2022 •

edited

yososs commented Mar 18, 2022 •

edited

mstr2 commented Mar 19, 2022

Maran23 commented Mar 19, 2022 •

edited

kevinrushforth commented Mar 19, 2022

openjdk bot commented Mar 19, 2022

nlisker left a comment

Maran23 commented Jul 1, 2022 •

edited

openjdk bot commented Jul 1, 2022 •

edited

Maran23 commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

kevinrushforth commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

8283346: Optimize observable ArrayList creation in FXCollections #758

8283346: Optimize observable ArrayList creation in FXCollections #758

Conversation

Maran23 commented Mar 17, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 17, 2022

mlbridge bot commented Mar 17, 2022 • edited

Webrevs

yososs commented Mar 18, 2022 • edited

mstr2 commented Mar 19, 2022

Maran23 commented Mar 19, 2022 • edited

kevinrushforth commented Mar 19, 2022

openjdk bot commented Mar 19, 2022

nlisker left a comment

Choose a reason for hiding this comment

Maran23 commented Jul 1, 2022 • edited

openjdk bot commented Jul 1, 2022 • edited

Maran23 commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

kevinrushforth commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

Maran23 commented Mar 17, 2022 •

edited by openjdk bot

mlbridge bot commented Mar 17, 2022 •

edited

yososs commented Mar 18, 2022 •

edited

Maran23 commented Mar 19, 2022 •

edited

Maran23 commented Jul 1, 2022 •

edited

openjdk bot commented Jul 1, 2022 •

edited