8264770: BidirectionalBinding should use InvalidationListener to prevent boxing

[mstr2]

The internal BidirectionalBinding class implements bidirectional bindings for JavaFX properties. The design intent of this class is to provide specializations for primitive value types to prevent boxing conversions (cf. specializations of the Property class with a similar design intent).

However, the primitive BidirectionalBinding implementations do not meet the design goal of preventing boxing conversions, because they implement ChangeListener.

ChangeListener is a generic SAM interface, which makes it impossibe to invoke an implementation of ChangeListener::changed with a primitive value (i.e. any primitive value will be auto-boxed).

The boxing conversion happens, as with all ChangeListeners, at the invocation site (for example, in ExpressionHelper). Since the boxing conversion has already happened by the time any of the BidirectionalBinding implementations is invoked, there's no point in using primitive specializations of BidirectionalBinding after the fact.

This issue can be solved by having BidirectionalBinding implement InvalidationListener instead, which by itself does not incur a boxing conversion. Because bidirectional bindings are eagerly evaluated, the observable behavior remains the same.

I've filed a bug report with the same title.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8264770: BidirectionalBinding should use InvalidationListener to prevent boxing

Reviewers

• Ambarish Rapte (@arapte - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/454/head:pull/454
$ git checkout pull/454

Update a local copy of the PR:
$ git checkout pull/454
$ git pull https://git.openjdk.java.net/jfx pull/454/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 454

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/454.diff

[bridgekeeper]

👋 Welcome back mstr2! 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.

[kevinrushforth]

I don't see this or any similar bug filed in our incident tracker. Did the submission complete to the point where you have an internal tracking number?

Is there a measurable benefit in doing this? For example, do you have a benchmark of some sort that shows garbage generation has been reduced or performance has been improved?

[mstr2]

Seems like I forgot to hit the send button on the webform. Here's the tracking number: 9069787.

I've used the following manual benchmark, which bidirectionally binds two properties and then produces a billion change notifications.

DoubleProperty property1 = new SimpleDoubleProperty();
DoubleProperty property2 = new SimpleDoubleProperty();
property2.bindBidirectional(property1);

long start = System.nanoTime();

for (int i = 0; i < 1000000000; ++i) {
    property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
}

long end = System.nanoTime();

System.out.println("Time elapsed (ms): " + (end - start) / 1000000);

And these are the results I got (time elapsed, in milliseconds):

	before	after
Test run 1:	28608	9122
Test run 2:	27928	9065
Test run 3:	31140	9181
Test run 4:	28041	9127
Test run 5:	28730	9181

So in this synthetic benchmark, the new implementation has a 3x performance improvement compared to the old implementation.

[kevinrushforth]

Seems like I forgot to hit the send button on the webform. Here's the tracking number: 9069787.

I see it now. And thanks for providing the benchmark. That's what I was looking for.

[kevinrushforth]

The bug is now visible here: https://bugs.openjdk.java.net/browse/JDK-8264770

[mlbridge]

Webrevs

• 01: Full - Incremental (538b9b38)
• 00: Full (1142087a)

[nlisker]

The benchmark might not tell the real story. To test these sorts of performance changes you have to use JMH. There's too much relating to JIT optimizations and JVM startup to be able to rely on the current benchmark.
The theoretical point about invalidation listeners not boxing in comparison to change listeners is sound, so it won't be worse, but if we are looking for performance metrics, they need to be proper ones.

[mstr2]

The benchmark might not tell the real story. To test these sorts of performance changes you have to use JMH. There's too much relating to JIT optimizations and JVM startup to be able to rely on the current benchmark.
The theoretical point about invalidation listeners not boxing in comparison to change listeners is sound, so it won't be worse, but if we are looking for performance metrics, they need to be proper ones.

While true, my main motivation for this issue was that the original code is wrongly implemented because it claims to do one thing, but doesn't do that thing. So it's not primarily a question of optimization, but of correctness of the implementation.

Anyway, here's a comparison benchmark done with JMH:

@State(Scope.Benchmark)
public class BindingBenchmark {
    DoubleProperty property1 = new SimpleDoubleProperty();
    DoubleProperty property2 = new SimpleDoubleProperty();

    public BindingBenchmark() {
        property2.bindBidirectional(property1);
    }

    @Benchmark
    public void benchmark() {
        for (int i = 0; i < 10000000; ++i) {
            property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
        }
    }
}

Benchmark	Mode	Cnt	Score	Error	Units
before	thrpt	5	3.455	0.029	ops/s
after	thrpt	5	10.322	0.790	ops/s

[kevinrushforth]

/reviewers 2

[openjdk]

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

[arapte]

looks all good to me

[kevinrushforth]

The fix looks good, but I noted two places where I think you need to initialize oldValue.

[mstr2]

I've added the two missing oldValue initializations.

[kevinrushforth]

Looks good (I note that the test failure on Linux is an unrelated bug that is under evaluation).

[openjdk]

@mstr2 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:

8264770: BidirectionalBinding should use InvalidationListener to prevent boxing

Reviewed-by: arapte, 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 59 new commits pushed to the master branch:

• 8ca7815: 8267160: Monocle mouse never get ENTERED state
• 9e8c617: 8266516: One label typo in the properties for bi-directional text
• 389e8c0: 8266968: Ignore test.com.sun.webkit.LocalStorageAccessTest
• e40b0b8: 8266811: Openjfx controls test build broken (Eclipse)
• c7833f1: 8266757: Add entry for jdk-12_doc-all.zip to gradle/verification-metadata.xml
• 3ac6bf0: 8266919: Gradle verification fails on windows x86
• eb913cb: 8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed
• ab7c151: 8262396: Update Mesa 3-D Headers to version 21.0.3
• 4b6c587: 8264157: Items of non-editable ComboBox cannot be selected using up/down keys
• 599ca1e: 8265277: SkinBase::registerChangeListener​ missing '@SInCE 9' javadoc tag
• ... and 49 more: https://git.openjdk.java.net/jfx/compare/e23a2feb8919285825993251b42acaaa13a41eb0...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 (@arapte, @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).

[mstr2]

/integrate

[openjdk]

@mstr2
Your change (at version 538b9b3) is now ready to be sponsored by a Committer.

[arapte]

/sponsor

[openjdk]

@arapte @mstr2 Since your change was applied there have been 59 commits pushed to the master branch:

• 8ca7815: 8267160: Monocle mouse never get ENTERED state
• 9e8c617: 8266516: One label typo in the properties for bi-directional text
• 389e8c0: 8266968: Ignore test.com.sun.webkit.LocalStorageAccessTest
• e40b0b8: 8266811: Openjfx controls test build broken (Eclipse)
• c7833f1: 8266757: Add entry for jdk-12_doc-all.zip to gradle/verification-metadata.xml
• 3ac6bf0: 8266919: Gradle verification fails on windows x86
• eb913cb: 8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed
• ab7c151: 8262396: Update Mesa 3-D Headers to version 21.0.3
• 4b6c587: 8264157: Items of non-editable ComboBox cannot be selected using up/down keys
• 599ca1e: 8265277: SkinBase::registerChangeListener​ missing '@SInCE 9' javadoc tag
• ... and 49 more: https://git.openjdk.java.net/jfx/compare/e23a2feb8919285825993251b42acaaa13a41eb0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 285a0b6.

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

[hjohn]

About lines 156-158:

    property1.setValue(property2.getValue());
    property1.addListener(binding);
    property2.addListener(binding);

The bindings added do not validate the properties anymore as it is an invalidation listener now instead of a change listener. This doesn't matter for property2 as its getValue is called which will force its revalidation, but for property1 this is not the case. This small program demonstrates this:

SimpleDoubleProperty p = new SimpleDoubleProperty(2);

InvalidationListener invalidationListener = obs -> System.out.println("Invalidated");

p.addListener(invalidationListener);

p.setValue(3);
p.setValue(4);

The program as expected only prints invalidated once.