On Fri, 8 Jul 2022 14:34:54 GMT, Nir Lisker <[email protected]> wrote:
>> I didn't get into the fine details of the GC discussion yet, but I have a >> few points I would like to share: >> >> 1. @hjohn's example 4, which seems to be a point of some disagreement, is a >> _de facto_ issue. Even if the technicalities or semantics imply that this is >> correct behavior, StackOverflow is littered with "why did my listener >> suddenly stopped working?" questions that are caused by this, and I have >> fell into this pitfall more than once myself (by starting from something >> like example 2, then making a slight change that transforms the code into >> something like example 4). >> I didn't look yet into the possible alternatives discussed here for >> dealing with this, but I do believe that going with "this behavior is >> technically correct" is not the best _practical_ decision, provided that >> changing it will be backwards compatible. >> 2. On the subject of `Disposer` and cleaning references in a background >> thread, the JDK has >> [Cleaner](https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/lang/ref/Cleaner.html) >> (there is a nice recent Inside Java >> [post](https://inside.java/2022/05/25/clean-cleaner/) about it), which seems >> to do what we need, at least abstractly. I didn't look at the code to see if >> it fits our needs exactly. It also uses a `ReferenceQueue` with >> `PhantomReference`s. >> 3. It is clear to me that whatever GC/referencing model we choose to go with >> eventually (with whatever combination of fluent vs. regular bindings, >> listeners vs. bindings, expressions vs. properties...), we will have to >> document what is expected of the user, and we will have to do it very well. >> Currently, there are some mentions in the docs of listeners holding strong >> references, and weak references being used in the implementation ("user >> beware"), but we will need to be a lot more explicit in my opinion. >> Something like @hjohn's set of examples or some snippets from this >> discussion would be a good starting point. If we don't want to commit to >> some detail, we should at least document the current behavior with a note >> that it might change. It's just too easy to mess this up (even before this >> PR was considered). I don't mind taking care of such a PR when an >> implementation is agreed upon. > >> Additionally, can you file a docs bug to clarify the GC behavior of bindings >> and mappings as suggested by @nlisker in point 3 of [this >> comment](https://github.com/openjdk/jfx/pull/675#issuecomment-1176975103)? > > @hjohn If the followup work on this issue is too much and you want to focus > on the implementation, you can assign it to me. > 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. ------------- PR: https://git.openjdk.org/jfx/pull/675
