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

[John Hendrikx]

On Fri, 8 Jul 2022 19:01:31 GMT, Michael Strauß <mstrauss 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

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

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