[jfx17u] RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
Clean backport to `jfx17u`. This has been tested along with other pending fixes in the `test-kcr-17.0.1` branch. - Commit messages: - 8273138: BidirectionalBinding fails to observe changes of invalid properties Changes: https://git.openjdk.java.net/jfx17u/pull/5/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=5=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273138 Stats: 54 lines in 2 files changed: 54 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx17u/pull/5.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/5/head:pull/5 PR: https://git.openjdk.java.net/jfx17u/pull/5
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Tue, 7 Sep 2021 19:43:36 GMT, Michael Strauß wrote: > > I think that restoring the existing behavior to fix this regression is > > important, and needs to be done regardless of what we might do in the > > future. > > This PR does that. Indeed it does. My comment was meant in reply to the point @nlisker raised. Sorry for not making that clear. - PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Tue, 7 Sep 2021 18:50:18 GMT, Kevin Rushforth wrote: > I think that restoring the existing behavior to fix this regression is > important, and needs to be done regardless of what we might do in the future. This PR does that. - PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
I think that restoring the existing behavior to fix this regression is
important, and needs to be done regardless of what we might do in the future.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Tue, 7 Sep 2021 10:53:48 GMT, Nir Lisker wrote: > There's an ongoing discussion about the eager evaluation of invalidation > listeners in the [mailing > list](http://mail.openjdk.java.net/pipermail/openjfx-dev/2021-September/031801.html). > Its conclusion might affect this patch, I didn't look at this one closely. The discussion is about whether adding a listener should validate the property. With this fix, both properties are validated in all relevant code paths, so it doesn't matter whether or not any of the properties is validated by adding an invalidation listener. - PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
There's an ongoing discussion about the eager evaluation of invalidation
listeners in the [mailing
list](http://mail.openjdk.java.net/pipermail/openjfx-dev/2021-September/031801.html).
Its conclusion might affect this patch, I didn't look at this one closely.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
OK I see. I didn't realize GA is due for tomorrow already.
I had September 14 in mind as a release date, but realize now that this is the
date for OpenJDK.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
Yes, it is far too late for 17 (code freeze was more than two weeks ago; GA is
tomorrow).
17.0.1 should be out in about 6 weeks, and this is a candidate for backporting.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
I see. This is unfortunate though, as this will definitely break some
applications in ways potentially difficult to diagnose.
Maybe this won't be too bad if the window until 17.0.1 is small, but still, I'm
afraid third party developers not following these discussions might be in for
some head scratching.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
[It seems to be too
late.](https://bugs.openjdk.java.net/browse/JDK-8273138?focusedCommentId=14445309=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14445309)
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
Am I correct in assuming that, as is, this PR is only going to be merged into
openJFX 18 ?
If so, because it addresses a regression introduced in jfx 17, we'll probably
want to have this one in 17 as well on day one, if its not too late.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
The explanation makes sense, and both the fix and the test look good. I can
confirm that the new test fails without the fix and passes with the fix.
-
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
> Should this be a new JBS ticket, or should we re-open 8264770 and associate
> this PR with it?
The former. We never reopen a bug that was resolved/fixed by a commit in the
repo.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
This change looks good to me.
-
PR: https://git.openjdk.java.net/jfx/pull/614
Re: RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
On Sun, 29 Aug 2021 04:12:22 GMT, Michael Strauß wrote:
> This PR fixes a bug that was introduced in #454.
>
> Since this fix might impact the performance considerations in the original
> PR, I ran a performance benchmark against the previous `ChangeListener`-based
> implementation, which still shows better performance for the new
> implementation:
>
>
> @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 < 1000; ++i) {
> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
> }
> }
> }
>
>
> | Benchmark | Mode | Cnt | Score | Error | Units |
> |---|--|-|---|---||
> | ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
> | InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
@kevinrushforth Should this be a new JBS ticket, or should we re-open
[8264770](https://bugs.openjdk.java.net/browse/JDK-8264770) and associate this
PR with it?
-
PR: https://git.openjdk.java.net/jfx/pull/614
RFR: 8273138: BidirectionalBinding fails to observe changes of invalid properties
This PR fixes a bug that was introduced in #454.
Since this fix might impact the performance considerations in the original PR,
I ran a performance benchmark against the previous `ChangeListener`-based
implementation, which still shows better performance for the new implementation:
@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 < 1000; ++i) {
property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
}
}
}
| Benchmark | Mode | Cnt | Score | Error | Units |
|---|--|-|---|---||
| ChangeListener | thrpt | 5 | 7.463 | 0.040 | ops/s |
| InvalidationListener (fixed) | thrpt | 5 | 15.095 | 0.092 | ops/s |
-
Commit messages:
- fixed invalidation bug
- added failing test
Changes: https://git.openjdk.java.net/jfx/pull/614/files
Webrev: https://webrevs.openjdk.java.net/?repo=jfx=614=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8273138
Stats: 54 lines in 2 files changed: 54 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jfx/pull/614.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/614/head:pull/614
PR: https://git.openjdk.java.net/jfx/pull/614
