Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 22:30:16 GMT, Michael Strauß wrote:
>> 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.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> added missing oldValue assignments
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 157:
> 155:
> 156: property1.setValue((T)property2.getValue());
> 157: property1.addListener(binding);
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.
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 22:30:16 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > added missing oldValue assignments Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 22:30:16 GMT, Michael Strauß wrote: >> 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. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > added missing oldValue assignments Looks good (I note that the test failure on Linux is an unrelated bug that is under evaluation). - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, Michael Strauß wrote: > 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. I've added the two missing `oldValue` initializations. - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
On Fri, 14 May 2021 21:40:39 GMT, Kevin Rushforth wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> added missing oldValue assignments
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
> line 585:
>
>> 583: private TypedGenericBidirectionalBinding(Property property1,
>> Property property2) {
>> 584: super(property1, property2);
>> 585: propertyRef1 = new WeakReference<>(property1);
>
> Don't you need to initialize `oldValue` here?
>
>
> oldValue = property1.get();
Yes, it should be initialized.
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing [v2]
> 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. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: added missing oldValue assignments - Changes: - all: https://git.openjdk.java.net/jfx/pull/454/files - new: https://git.openjdk.java.net/jfx/pull/454/files/1142087a..538b9b38 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=454=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=454=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/454.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/454/head:pull/454 PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, Michael Strauß wrote:
> 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.
The fix looks good, but I noted two places where I think you need to initialize
`oldValue`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 585:
> 583: private TypedGenericBidirectionalBinding(Property property1,
> Property property2) {
> 584: super(property1, property2);
> 585: propertyRef1 = new WeakReference<>(property1);
Don't you need to initialize `oldValue` here?
oldValue = property1.get();
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 657:
> 655: private TypedNumberBidirectionalBinding(Property property1,
> Property property2) {
> 656: super(property1, property2);
> 657: propertyRef1 = new WeakReference<>(property1);
Same comment as above. Shouldn't `oldValue` be set?
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, mstr2 wrote: > 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. looks all good to me - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Tue, 6 Apr 2021 14:29:28 GMT, Nir Lisker wrote:
> 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 < 1000; ++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 |
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Tue, 6 Apr 2021 12:08:39 GMT, Kevin Rushforth wrote: >>> 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. > > The bug is now visible here: https://bugs.openjdk.java.net/browse/JDK-8264770 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. - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 21:54:40 GMT, Kevin Rushforth wrote:
>> 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.
>
> 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?
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 < 10; ++i) {
property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
}
long end = System.nanoTime();
System.out.println("Time elapsed (ms): " + (end - start) / 100);
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.
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 23:38:29 GMT, Kevin Rushforth wrote:
>> 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 < 10; ++i) {
>> property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
>> }
>>
>> long end = System.nanoTime();
>>
>> System.out.println("Time elapsed (ms): " + (end - start) / 100);
>>
>> 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.
>
>> 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.
The bug is now visible here: https://bugs.openjdk.java.net/browse/JDK-8264770
-
PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Mon, 5 Apr 2021 23:16:53 GMT, mstr2 wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/454
Re: RFR: 8264770: BidirectionalBinding should use InvalidationListener to prevent boxing
On Sat, 3 Apr 2021 15:20:41 GMT, mstr2 wrote: > 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. 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? - PR: https://git.openjdk.java.net/jfx/pull/454
