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

[Michael Strauß]

On Fri, 8 Jul 2022 14:34:54 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> I didn't get into the fine details of the GC discussion yet, but I have a few points I would like to share:
>> 
>> 1. @hjohn's example 4, which seems to be a point of some disagreement, is a _de facto_ issue. Even if the technicalities or semantics imply that this is correct behavior, StackOverflow is littered with "why did my listener suddenly stopped working?" questions that are caused by this, and I have fell into this pitfall more than once myself (by starting from something like example 2, then making a slight change that transforms the code into something like example 4).
>>   I didn't look yet into the possible alternatives discussed here for dealing with this, but I do believe that going with "this behavior is technically correct" is not the best _practical_ decision, provided that changing it will be backwards compatible.
>> 2. On the subject of `Disposer` and cleaning references in a background thread, the JDK has [Cleaner](https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/ref/Cleaner.html) (there is a nice recent Inside Java [post](https://inside.java/2022/05/25/clean-cleaner/) about it), which seems to do what we need, at least abstractly. I didn't look at the code to see if it fits our needs exactly. It also uses a `ReferenceQueue` with `PhantomReference`s.
>> 3. It is clear to me that whatever GC/referencing model we choose to go with eventually (with whatever combination of fluent vs. regular bindings, listeners vs. bindings, expressions vs. properties...), we will have to document what is expected of the user, and we will have to do it very well. Currently, there are some mentions in the docs of listeners holding strong references, and weak references being used in the implementation ("user beware"), but we will need to be a lot more explicit in my opinion. Something like @hjohn's set of examples or some snippets from this discussion would be a good starting point. If we don't want to commit to some detail, we should at least document the current behavior with a note that it might change. It's just too easy to mess this up (even before this PR was considered). I don't mind taking care of such a PR when an implementation is agreed upon.
>
>> Additionally, can you file a docs bug to clarify the GC behavior of bindings and mappings as suggested by @nlisker in point 3 of [this comment](https://github.com/openjdk/jfx/pull/675#issuecomment-1176975103)?
> 
> @hjohn If the followup work on this issue is too much and you want to focus on the implementation, you can assign it to me.

> I've done some experimenting using a `Cleaner` suggested by @nlisker;
> [...]
> From what I can see, we'd need to change all locations where `bind` is implemented using a weak listener, and add a `Cleaner`. 

Yes, this must be implemented for all `*PropertyBase` classes. We can save a few bytes for non-`ConcurrentListenerAccess` observables with an implementation like this:

private static class Listener implements InvalidationListener, WeakListener {
    private final WeakReference<StringPropertyBase> wref;
    private final Cleaner.Cleanable cleanable;

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

        if (observable instanceof ConcurrentListenerAccess) {
            cleanable = CLEANER.register(ref, this::removeListener);
        } else {
            cleanable = null;
        }
    }

    // Note: dispose() must be called in StringPropertyBase.unbind()
    public void dispose() {
        if (cleanable != null) {
            cleanable.clean();
        } else {
            removeListener();
        }
    }

    private void removeListener() {
        StringPropertyBase ref = wref.get();
        if (ref != null) {
            ref.observable.removeListener(this);
        }
    }

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

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



> 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).

Not sure I'm following here. Do you want to implement this pattern for all property implementations? If we just want to implement it for mapped bindings, only the `LazyObjectBinding` class needs to be thread-safe.

Note that it's not enough for the `addListener` and `removeListener` methods to be `synchronized`. _All_ reads and writes must be protected, which (in the case of `LazyObjectBinding`) includes the `invalidate` and `isObserved` methods.

But if we do that, the lock tax must be paid on every single access to the `ExpressionHelper` field (because `ExpressionHelper` is not thread-safe). Locking any and all usages of `ExpressionHelper` can have performance implications that we need to evaluate carefully.

However, I think we might get away with an implementation that only requires locking for the `addListener`/`removeListener` methods, but (crucially) not for `fireValueChangedEvent`:

private volatile ConcurrentExpressionHelper<T> helper = null;

@Override
public synchronized void addListener(InvalidationListener listener) {
    helper = ConcurrentExpressionHelper.addListener(helper, this, listener);
}

@Override
public synchronized void removeListener(InvalidationListener listener) {
    helper = ConcurrentExpressionHelper.removeListener(helper, listener);
}

@Override
public synchronized void addListener(ChangeListener<? super T> listener) {
    helper = ConcurrentExpressionHelper.addListener(helper, this, listener);
}

@Override
public synchronized void removeListener(ChangeListener<? super T> listener) {
    helper = ConcurrentExpressionHelper.removeListener(helper, listener);
}

protected void fireValueChangedEvent() {
    ConcurrentExpressionHelper.fireValueChangedEvent(helper);
}


This implementation works by creating immutable specializations of [ConcurrentExpressionHelper](https://gist.github.com/mstr2/1efc9e866f81622253711a963bd272fc). When a listener is added, a new `ConcurrentExpressionHelper` instance is created, which doesn't interfere with an existing instance that is currently in use by `ConcurrentExpressionHelper.fireValueChangedEvent`. The`ConcurrentExpressionHelper.Generic` specialization is mutable, but uses the lock-free `ConcurrentLinkedQueue` to store its listeners.

`ConcurrentExpressionHelper` avoids locking the (probably most frequently invoked) `fireValueChangedEvent` method, but sacrifices cache locality when a large number of listeners is added. We should probably benchmark all of these proposals before deciding on a solution.

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

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