RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v18]

[John Hendrikx]

On Fri, 1 Jul 2022 15:16:24 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> 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
>>       ));
>> 
>>       // Standard JavaFX + Optional:
>>       label.textProperty().bind(Bindings.createStringBinding(
>>           () -> Optinal.ofNullable(employee.get())
>>               .map(Employee::getCompany)
>>               .map(Company::getName)
>>               .orElse(""),
>>          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 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 27 additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into feature/fluent-bindings
>  - Add null checks in Subscription
>  - Update copyrights
>  - Move private binding classes to com.sun.javafx.binding package
>  - Add note to Bindings#select to consider ObservableValue#flatMap
>  - Fix bug invalidation bug in FlatMappedBinding
>    
>    Also fixed a secondary issue where the indirect source of the binding
>    was unsubscribed and resubscribed each time its value was recomputed.
>    
>    Add additional comments to clarify how FlatMappedBinding works.
>    
>    Added test cases for these newly discovered issues.
>  - Fix typos in LazyObjectBinding
>  - Rename observeInputs to observeSources
>  - Expand flatMap javadoc with additional wording from Optional#flatMap
>  - Add since tags to all new API
>  - ... and 17 more: https://git.openjdk.org/jfx/compare/fc59ad03...d66f2ba6

> Going a step further, I think the idea of actively disposing dead listeners can help us solve this problem. But there's a catch: we can't just assume that we can safely call `removeListener` from a disposer thread (nor from the JavaFX application thread, for that matter).
> 
> There is no requirement that an `ObservableValue` implementation be safe to use with regards to concurrent reads and writes. In order to remove a listener on an arbitrary thread, we need to introduce a stronger thread-safety guarantee than we currently have. We can't do that in general, as there are countless `ObservableValue` implementations that would be broken after this change. But we _can_ do it for mapped bindings (or any built-in binding implementations).
>  
> Now we can use the existing `Disposer` mechanism to automatically schedule listeners to be removed from `ConcurrentListenerAccess` implementations when the bound property is eligible for collection.

I've done some experimenting using a `Cleaner` suggested by @nlisker; I changed the private `WeakListener` class inside of `StringPropertyBase` and then added `synchronized` to all its `addListener` and `removeListener` methods:

    private static class Listener implements InvalidationListener, WeakListener {
        private static final Cleaner CLEANER = Cleaner.create();

        private final WeakReference<StringPropertyBase> wref;
        private final Cleanable cleanable;

        public Listener(StringPropertyBase ref, Observable observable) {
            this.wref = new WeakReference<>(ref);
            
            Cleanable c = () -> observable.removeListener(this);

            this.cleanable = observable instanceof ConcurrentListenerAccess ? CLEANER.register(ref, c::clean) : c;
        }

        @Override
        public void invalidated(Observable observable) {
            StringPropertyBase ref = wref.get();
            if (ref == null) {
                cleanable.clean();
            } else {
                ref.markInvalid();
            }
        }

        @Override
        public boolean wasGarbageCollected() {
            return wref.get() == null;
        }
    }

Doing 100.000 throwaway binds on a single property, this mechanism manages to keep up quite well using standard GC settings. Within seconds, all 100.000 stubs have been cleaned up (note: 100.000 is already an insane amount).  If I go higher, the cleaning mechanism can't quite keep up, but that's primarily because the operation becomes quite expensive due to the large listener list involved (cleaning doesn't start immediately, and if the list of listeners managed by `ExpressionHelper` has grown close to a million items, removing items one by one is quite slow).

> It's sufficient to implement `ConcurrentListenerAccess` for `LazyObjectBinding` to make `map` and `flatMap` work the way I'd expect it to work.

>From what I can see, we'd need to change all locations where `bind` is implemented using a weak listener, and add a `Cleaner`. Those same classes need their add/remove listener methods synchronized (putting `synchronized` on the static helper methods in `ExpressionListener` doesn't look like a good idea as it would block listener changes to unrelated properties).

If there is agreement on this approach, I could create a separate MR for this (since it could be implemented and merged before this one) or integrate the changes into this one.

-------------

PR: https://git.openjdk.org/jfx/pull/675