Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v3]
On Wed, 5 Jan 2022 18:14:44 GMT, Florian Kirmaier wrote:
>> Making the initial listener of the ListProperty weak fixes the problem.
>> The same is fixed for Set and Map.
>> Due to a smart implementation, this is done without any performance drawback.
>> (The trick is to have an object, which is both the WeakReference and the
>> Changelistener)
>> By implying the same trick to the InvalidationListener, this should even
>> improve the performance of the collection properties.
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8277848
> Further optimization based code review.
> This Bugfix should now event improve the performance
modules/javafx.base/src/main/java/javafx/beans/property/ListPropertyBase.java
line 338:
> 336: public void onChanged(Change change) {
> 337: ListPropertyBase ref = get();
> 338: if(ref != null) {
Minor: space after `if`
modules/javafx.base/src/main/java/javafx/beans/property/MapPropertyBase.java
line 339:
> 337: public void onChanged(Change change) {
> 338: MapPropertyBase ref = get();
> 339: if(ref != null) {
Minor: space after `if`
modules/javafx.base/src/main/java/javafx/beans/property/SetPropertyBase.java
line 341:
> 339: public void onChanged(Change change) {
> 340: SetPropertyBase ref = get();
> 341: if(ref != null) {
Minor: space after `if`
-
PR: https://git.openjdk.java.net/jfx/pull/689
Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]
On Fri, 4 Jun 2021 05:04:17 GMT, Michael Strauß wrote: >> This PR fixes an issue when building OpenJFX on Windows and command-line >> arguments contain paths with spaces. >> >> The problem is that on Windows, `ant` is invoked via `cmd`, which leads to >> quotes being interpreted twice. This can be fixed with the option `cmd /s`, >> which prevents interpreting quotes on the rest of the command line. The >> result is as if the rest of the command line had been typed verbatim at the >> command prompt. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into fixes/JDK-8267059 > - Use cmd /s option when building on Windows @johanvos or @tiainen can you comment as to whether you have any concerns with this? It seems a safe fix to me. - PR: https://git.openjdk.java.net/jfx/pull/499
Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v2]
On Fri, 31 Dec 2021 12:53:54 GMT, Michael Strauß wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8277848
>> Added missing change
>
> Why are the new listener imlementations called `BaseChangeListener` and
> `BaseInvalidationListener`, i.e. why the _Base_?
>
> Also, if you're going to the trouble of refactoring the existing listener
> implementation, have you considered merging the very similar implementations
> into a single class? You can then re-use the listener instance and save
> another object allocation in this way:
>
>
> private static class Listener extends WeakReference>
> implements InvalidationListener, ListChangeListener, WeakListener {
> Listener(ListPropertyBase ref) {
> super(ref);
> }
>
> @Override
> public boolean wasGarbageCollected() {
> return get() == null;
> }
>
> @Override
> public void onChanged(Change change) {
> ListPropertyBase ref = get();
> if(ref != null) {
> ref.invalidateProperties();
> ref.invalidated();
> ref.fireValueChangedEvent(change);
> }
> }
>
> @Override
> public void invalidated(Observable observable) {
> ListPropertyBase ref = get();
> if (ref == null) {
> observable.removeListener(this);
> } else {
> ref.markInvalid(ref.value);
> }
> }
> }
@mstr2
Great point. The chosen name was just because I needed a name. I switched now
to the name "Listener".
I've now merged the ChangeListener with the Invalidation Listener, as you've
suggested.
The PR should now improve the performance while fixing a bug. It might even be
quite relevant because these classes are used very often.
-
PR: https://git.openjdk.java.net/jfx/pull/689
Re: RFR: 8277848 Binding and Unbinding to List leads to memory leak [v3]
> Making the initial listener of the ListProperty weak fixes the problem. > The same is fixed for Set and Map. > Due to a smart implementation, this is done without any performance drawback. > (The trick is to have an object, which is both the WeakReference and the > Changelistener) > By implying the same trick to the InvalidationListener, this should even > improve the performance of the collection properties. Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision: JDK-8277848 Further optimization based code review. This Bugfix should now event improve the performance - Changes: - all: https://git.openjdk.java.net/jfx/pull/689/files - new: https://git.openjdk.java.net/jfx/pull/689/files/f9b7009b..ec90b3d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=689=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=689=01-02 Stats: 106 lines in 3 files changed: 15 ins; 63 del; 28 mod Patch: https://git.openjdk.java.net/jfx/pull/689.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/689/head:pull/689 PR: https://git.openjdk.java.net/jfx/pull/689
Re: RFR: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces [v2]
On Fri, 27 Aug 2021 13:39:13 GMT, Kevin Rushforth wrote: >> Michael Strauß has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Merge branch 'master' into fixes/JDK-8267059 >> - Use cmd /s option when building on Windows > > @tiainen or @arapte can one of you be the second reviewer on this? @kevinrushforth I think this is a very useful fix that has been tested by three people in the last months (of which only your review counts for Skara). Can you lower the number of required reviews to 1 in order for this PR to move forward to integration? - PR: https://git.openjdk.java.net/jfx/pull/499
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Tue, 4 Jan 2022 03:28:57 GMT, Nir Lisker wrote:
> Unrelated to the review, will it makes sense in the future to make all
> bindings lazily register listeners like `LazyObjectBinding`, perhaps when we
> introduce `Subscription`?
That would need to be very well tested. There are some noticeable differences
in behavior vs the standard bindings:
1) Standard bindings can more easily be GC'd (accidentally usually, but it
could be intentional), take for example:
textProperty.addListener((obs, old, current) ->
System.out.println(current));
textProperty.concat("A").addListener((obs, old, current) ->
System.out.println(current));
These behave very different. The first listener keeps working as you'd expect,
while the second one can stop working as soon the GC runs. This is because
`concat` results in an `ObservableValue` that is weakly bound. Compare this to:
textProperty.map(x -> x + "A").addListener((obs, old, current) ->
System.out.println(current));
This keeps working and will not be GC'd by accident.
2) Standard bindings will always cache values. This means that when `getValue`
is called, it will just return the cached value instead of calling
`computeValue`. If `computeValue` is really expensive (unwise since this
happens on the FX thread) then this cost is paid each time for Lazy bindings,
at least when they're not bound to anything else (unobserved) and you are just
using `getValue`. Furthermore, for a chain of Lazy bindings that is unobserved,
this would propagate through the entire chain. As soon as they're observed
though, they become non-lazy and values are cached.
In effect, Lazy bindings behave the same as standard bindings when they're
observed but their behavior changes when not observed: they never become valid
and they stop caching values
This has pros and cons:
Pro: Lazy bindings can be garbage collected when not referenced and not
actively being used without the use of weak listeners. See the first example
where the binding keeps working when used by `println` lambda. This is in
contrast to traditional bindings which can be garbage collected when
unreferenced by user code even if actively used(!!). This is a huge gotcha and
one of the biggest reasons to use the lazy model.
Pro: Lazy bindings don't register unnecessary listeners to be aware of changed
or invalidated values that are not used by anyone. A good example is the
problems we saw about a year ago where `Node` had created an `isShowing`
property which bounds to its `Scene` and `Window`. This looks harmless, until
you realize that a listener is created on these properties for each `Node` in
existence. If a `Scene` has tens of thousands of `Node`s then that means that
the `Scene#windowProperty` will have a listener list containing tens of
thousands of entries. This list is not only expensive to change (when a node
is added or removed) but also expensive to act on (when the scene, window or
its showing state changes). And for what? Almost nobody was actually using
this property, yet listeners had to be added for each and every `Node`. In
effect with lazy bindings, it is much cheaper now to create properties that
create convenient calculated values which will only register listeners or
compute
their values when in actual use.
Con: lazy bindings never become valid when not observed. This means that
`getValue` will always have to recompute the value as the value is not cached.
However, if you register an invalidation listener the binding becomes observed
and it will start behaving like a traditional binding sending invalidation
events and caching the current value. A small "pro" here could be that this
means that intermediate values in a long binding chain don't take up memory
(and are prevented from GC), however that goes away as soon as the binding is
observed.
In summary: I think lazy bindings are far superior in the experience that they
offer, but it does come at a cost that values may need to be recomputed every
time when the bindings are unobserved. However, doing substantial work in
`computeValue` is probably unwise anyway as it blocks the FX thread so making
lazy binding the default behavior in well behaving code could be of only minor
impact.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 5 Jan 2022 12:08:01 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
>> line 188:
>>
>>> 186: * {@code ObservableValue} given by applying {@code mapper} on
>>> the value
>>> 187: * held by this {@code ObservableValue}, and is {@code null}
>>> when this
>>> 188: * {@code ObservableValue} holds {@code null}, never null
>>
>> an {@code ObservableValue} that holds the value of the {@code
>> ObservableValue} resulting
>> from applying a mapping on the value held by this {@code ObservableValue};
>> never {@code null} itself
>
> How about:
>
> an {@code ObservableValue} holding the value of an {@code ObservableValue}
> resulting from a mapping of this {@code ObservableValue}'s value or
> holds {@code null} when the value is {@code null}; never returns {@code
> null}
>
> They are tough to describe; if you don't like the `or holds {@code null}`
> parts I can remove those from all the functions.
Here's another attempt:
a new {@link ObservableValue} instance that holds the value that was obtained by
applying the mapping function on the value held by this {@code ObservableValue}.
If this {@code ObservableValue} holds {@code null}, the new {@code
ObservableValue}
will also hold {@code null}.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 5 Jan 2022 09:52:29 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java
>> line 204:
>>
>>> 202: *
>>> 203: * @return {@code true} if this binding is allowed to become
>>> valid, otherwise
>>> 204: * {@code false}
>>
>> Typo: overriden -> overridden
>>
>> I would add a a first-sentence summary and an explanation as to why a
>> binding would not allow it. I would write something like
>>
>> Checks if the binding is allowed to become valid. Overriding classes can
>> prevent a binding from becoming
>> valid. This is useful when .
>>
>> The default implementation always allows bindings to become valid.
>
> I've made your suggested changes and added this explanation: "This is useful
> in subclasses which do not always listen for invalidations of their
> dependencies and prefer to recompute the current value instead. This can also
> be useful if caching of the current computed value is not desirable."
>
> Furthermore, I noticed I forgot to make the code changes that prevent caching
> of the value when the binding is invalid -- bindings currently cache their
> value even when invalid, which could lead to situations where something is
> still being referenced in an invalid binding that should have been GC'd.
Something like that was previously discussed in
https://github.com/javafxports/openjdk-jfx/pull/110.
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Wed, 5 Jan 2022 09:45:21 GMT, John Hendrikx wrote:
>> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java
>> line 193:
>>
>>> 191: *
>>> 192: * @return {@code true} when this binding currently has one or more
>>> 193: * listeners, otherwise {@code false}
>>
>> Maybe the method description
>> `Checks if the binding has at least one listener registered on it.`
>> is more straightforward, especially since it is a first-sentence summary.
>> The `@return` description can contain the info on what is returned. As for
>> that, maybe
>> `{@code true} if this binding currently has one or more listeners registered
>> on it, otherwise {@code false}`
>> is more precise.
>> I'm not sure if "registered on" or "registered to" is better, not that I
>> think it matters.
>>
>> I would also like to see an explanation of how the method should/can be used
>> by subclasses, but it looks to be tied to `Subscription`, which isn't added
>> yet, so I don't have a good idea on this.
>
> It is not strictly tied to `Subscription`, the method is required to
> determine when `LazyObjectBinding` must register listeners on its
> dependencies and when it can remove them again (basically when it stops being
> lazy or when it can become lazy again).
I've added "This is useful for subclasses which want to conserve resources when
not observed."
-
PR: https://git.openjdk.java.net/jfx/pull/675
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v3]
On Sun, 2 Jan 2022 20:18:02 GMT, Nir Lisker wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Upgrade tests to JUnit 5
>
> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java
> line 193:
>
>> 191: *
>> 192: * @return {@code true} when this binding currently has one or more
>> 193: * listeners, otherwise {@code false}
>
> Maybe the method description
> `Checks if the binding has at least one listener registered on it.`
> is more straightforward, especially since it is a first-sentence summary. The
> `@return` description can contain the info on what is returned. As for that,
> maybe
> `{@code true} if this binding currently has one or more listeners registered
> on it, otherwise {@code false}`
> is more precise.
> I'm not sure if "registered on" or "registered to" is better, not that I
> think it matters.
>
> I would also like to see an explanation of how the method should/can be used
> by subclasses, but it looks to be tied to `Subscription`, which isn't added
> yet, so I don't have a good idea on this.
It is not strictly tied to `Subscription`, the method is required to determine
when `LazyObjectBinding` must register listeners on its dependencies and when
it can remove them again (basically when it stops being lazy or when it can
become lazy again).
> modules/javafx.base/src/main/java/javafx/beans/binding/ObjectBinding.java
> line 204:
>
>> 202: *
>> 203: * @return {@code true} if this binding is allowed to become valid,
>> otherwise
>> 204: * {@code false}
>
> Typo: overriden -> overridden
>
> I would add a a first-sentence summary and an explanation as to why a binding
> would not allow it. I would write something like
>
> Checks if the binding is allowed to become valid. Overriding classes can
> prevent a binding from becoming
> valid. This is useful when .
>
> The default implementation always allows bindings to become valid.
I've made your suggested changes and added this explanation: "This is useful in
subclasses which do not always listen for invalidations of their dependencies
and prefer to recompute the current value instead. This can also be useful if
caching of the current computed value is not desirable."
Furthermore, I noticed I forgot to make the code changes that prevent caching
of the value when the binding is invalid -- bindings currently cache their
value even when invalid, which could lead to situations where something is
still being referenced in an invalid binding that should have been GC'd.
> modules/javafx.base/src/main/java/javafx/beans/value/MappedBinding.java line
> 34:
>
>> 32:
>> 33: class MappedBinding extends LazyObjectBinding {
>> 34: private final ObservableValue source;
>
> We usually have an empty line below the class declaration. Not sure if this
> is enforced. Same for the other classes.
I've added them everywhere.
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 146:
>
>> 144: * Returns an {@link ObservableValue} which provides a mapping of
>> the value
>> 145: * held by this {@code ObservableValue}, and provides {@code null}
>> when this
>> 146: * {@code ObservableValue} holds {@code null}.
>
> No need to `@link` since we are in the same class already.
>
> While this description is very similar to that of ReactFX, I would prefer a
> shorter summary of what the method does as its first sentence, and this will
> allow to unpack the details more cleanly. Maybe something like:
>
> Creates an {@code ObservableValue} that holds the result of applying a
> mapping on the value held
> by this {@code ObservableValue}. The result is updated (lazily) when the
> value held by this
> {@code ObservableValue} changes. If this value is {@code null}, the resulting
> value is also {@code null}
> (the mapping is not applied).
>
> For example, mapping a lower case string to an uppercase string, and to a
> check if the string is blank:
>
> var lowercase = new SimpleStringProperty("abcd");
> ObservableValue uppercase = lowercase.map(String::toUpperCase);
> // "ABCD"
> ObservableValue blank = lowercase.map(String::isBlank);
> // false
> lowercase.set(" ");
> uppercase.getValue(); // " "
> blank.getValue(); // true
>
Copied these suggestions with some slight modifications, and I simplified the
example a little bit.
> modules/javafx.base/src/main/java/javafx/beans/value/ObservableValue.java
> line 149:
>
>> 147: *
>> 148: * @param the type of values held by the resulting {@code
>> ObservableValue}
>> 149: * @param mapper a {@link Function} which converts a given value to
>> a new value, cannot be null
>
> No need to `@link` since its linked in the method argument list when the docs
> are generated.
> `null` should be in `@code`.
>
> Worth thinking about adding the types of the
Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v4]
> This is an implementation of the proposal in
> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
> (@nlisker) have been working on. It's a complete implementation including
> good test coverage.
>
> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
> API footprint. Compared to the PoC this is lacking public API for
> subscriptions, and does not include `orElseGet` or the `conditionOn`
> conditional mapping.
>
> **Flexible Mappings**
> Map the contents of a property any way you like with `map`, or map nested
> properties with `flatMap`.
>
> **Lazy**
> The bindings created are lazy, which means they are always _invalid_ when not
> themselves observed. This allows for easier garbage collection (once the last
> observer is removed, a chain of bindings will stop observing their parents)
> and less listener management when dealing with nested properties.
> Furthermore, this allows inclusion of such bindings in classes such as `Node`
> without listeners being created when the binding itself is not used (this
> would allow for the inclusion of a `treeShowingProperty` in `Node` without
> creating excessive listeners, see this fix I did in an earlier PR:
> https://github.com/openjdk/jfx/pull/185)
>
> **Null Safe**
> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
> when the value they would be mapping is `null`. This makes mapping nested
> properties with `flatMap` trivial as the `null` case does not need to be
> taken into account in a chain like this:
> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
> Instead a default can be provided with `orElse`.
>
> Some examples:
>
> void mapProperty() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(() ->
> text.getValueSafe().toUpperCase(), text));
>
> // Fluent: much more compact, no need to handle null
> label.textProperty().bind(text.map(String::toUpperCase));
> }
>
> void calculateCharactersLeft() {
> // Standard JavaFX:
>
> label.textProperty().bind(text.length().negate().add(100).asString().concat("
> characters left"));
>
> // Fluent: slightly more compact and more clear (no negate needed)
> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() + "
> characters left"));
> }
>
> void mapNestedValue() {
> // Standard JavaFX:
> label.textProperty().bind(Bindings.createStringBinding(
> () -> employee.get() == null ? ""
> : employee.get().getCompany() == null ? ""
> : employee.get().getCompany().getName(),
> employee
> ));
>
> // Fluent: no need to handle nulls everywhere
> label.textProperty().bind(
> employee.map(Employee::getCompany)
> .map(Company::getName)
> .orElse("")
> );
> }
>
> void mapNestedProperty() {
> // Standard JavaFX:
> label.textProperty().bind(
> Bindings.when(Bindings.selectBoolean(label.sceneProperty(), "window",
> "showing"))
> .then("Visible")
> .otherwise("Not Visible")
> );
>
> // Fluent: type safe
> label.textProperty().bind(label.sceneProperty()
> .flatMap(Scene::windowProperty)
> .flatMap(Window::showingProperty)
> .orElse(false)
> .map(showing -> showing ? "Visible" : "Not Visible")
> );
> }
>
> Note that this is based on ideas in ReactFX and my own experiments in
> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
> that this is much better directly integrated into JavaFX, and I'm hoping this
> proof of concept will be able to move such an effort forward.
John Hendrikx has updated the pull request incrementally with one additional
commit since the last revision:
Apply changes suggested in review and updated copyright years to 2022
-
Changes:
- all: https://git.openjdk.java.net/jfx/pull/675/files
- new: https://git.openjdk.java.net/jfx/pull/675/files/312fb506..30e8ceac
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jfx=675=03
- incr: https://webrevs.openjdk.java.net/?repo=jfx=675=02-03
Stats: 182 lines in 11 files changed: 127 ins; 13 del; 42 mod
Patch: https://git.openjdk.java.net/jfx/pull/675.diff
Fetch: git fetch https://git.openjdk.java.net/jfx pull/675/head:pull/675
PR: https://git.openjdk.java.net/jfx/pull/675
