On Fri, 8 Jul 2022 21:00:39 GMT, John Hendrikx <[email protected]> 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
-------------
PR: https://git.openjdk.org/jfx/pull/675
