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

[John Hendrikx]

On Fri, 8 Jul 2022 21:00:39 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>>> 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.
>
>> 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.
> 
> Yes, true, only making it thread-safe for that class should remove a chain of fluent bindings.  I think however that it would be good to implement this for as many classes as we can as the stub cleaning is normally only triggered on invalidation/changes (and as I recently discovered, when `ExpressionHelper` resizes its backing list).
> 
>> 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.
> 
> Yes, true, I only fixed synchronization issues in my experiment and didn't look much further than that yet.
> 
>> 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.
> 
> Yeah, we shouldn't do that, it synchronizes all accesses to all property lists everywhere, it would be an easy "fix" as it's in one place, but it doesn't feel right.
> 
>> However, I think we might get away with an implementation that only requires locking for the `addListener`/`removeListener` methods, but (crucially) not for `fireValueChangedEvent`:
>> 
>> 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.
> 
> I see you have been playing with improving `ExpressionHelper` as well :) I noticed there is a bug in the current one, where a copy of the list is made each time when `locked` is `true`, even when the list was already copied for the "current" lock.  I ran into this problem right after I fixed the performance issue (it wasn't obvious before as it was already very slow, but basically an extra copy is happening in some circumstances on top of the array copying that is done to remove an entry).
> 
> The situation where `locked` is true doesn't happen that often for normal code, but it happens specifically in the case when a call to `invalidated` is cleaning up dead stubs...
> 
>> `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.
> 
> Yes, I think that's a good idea, I considered using `ConcurrentLinkedQueue` as well since we don't need a structure with index based access, but I couldn't find one with good O performance for `remove(T)` that wouldn't subtly change the current semantics.  FWIW, here's the quick `Collection` implementation I whipped up: https://gist.github.com/hjohn/8fee1e5d1a9eacbbb3e021f8a37f582b
> 
> And the changes in `ExpressionHelper` (including different locking): https://gist.github.com/hjohn/f88362ea78adef96f3a54d97e2405076

> @hjohn Sorry to append message here, but I don't know other places where we could talk about this topic Is it a good idea if Binding class provide method like this? Inspired by this PR

@chengenzhao The only way to do this right now using the fluent bindings is like this:

    Rectangle rect = new Rectangle();

    rect.widthProperty()
      .flatMap(x -> rect.heightProperty())
      .map(y -> rect.getWidth() * rect.getHeight())
      .addListener((obs, old, a) -> System.out.println("Area = " + a));

It is not very pretty.

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

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