On Fri, 6 Jan 2023 19:34:17 GMT, John Hendrikx <jhendr...@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
