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

[Chengen Zhao]

On Fri, 6 Jan 2023 19:34:17 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

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

@hjohn 
Is it that what we really need here is a reduce function?
since we already have map(thanks to this PR) then we probably need a reduce function to zip those map products?
e.g.

textProperty.bind(Binding.reduce(widthProperty, heightProperty, (w, h) -> "Area is "+w.doubleValue()*h.doubleValue()));

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

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