Re: RFR: 8274771: Map, FlatMap and OrElse fluent bindings for ObservableValue [v14]
On Tue, 22 Mar 2022 07:46:40 GMT, John Hendrikx wrote:
>> This is an implementation of the proposal in
>> https://bugs.openjdk.java.net/browse/JDK-8274771 that me and Nir Lisker
>> (@nlisker) have been working on. It's a complete implementation including
>> good test coverage.
>>
>> This was based on https://github.com/openjdk/jfx/pull/434 but with a smaller
>> API footprint. Compared to the PoC this is lacking public API for
>> subscriptions, and does not include `orElseGet` or the `conditionOn`
>> conditional mapping.
>>
>> **Flexible Mappings**
>> Map the contents of a property any way you like with `map`, or map nested
>> properties with `flatMap`.
>>
>> **Lazy**
>> The bindings created are lazy, which means they are always _invalid_ when
>> not themselves observed. This allows for easier garbage collection (once the
>> last observer is removed, a chain of bindings will stop observing their
>> parents) and less listener management when dealing with nested properties.
>> Furthermore, this allows inclusion of such bindings in classes such as
>> `Node` without listeners being created when the binding itself is not used
>> (this would allow for the inclusion of a `treeShowingProperty` in `Node`
>> without creating excessive listeners, see this fix I did in an earlier PR:
>> https://github.com/openjdk/jfx/pull/185)
>>
>> **Null Safe**
>> The `map` and `flatMap` methods are skipped, similar to `java.util.Optional`
>> when the value they would be mapping is `null`. This makes mapping nested
>> properties with `flatMap` trivial as the `null` case does not need to be
>> taken into account in a chain like this:
>> `node.sceneProperty().flatMap(Scene::windowProperty).flatMap(Window::showingProperty)`.
>> Instead a default can be provided with `orElse`.
>>
>> Some examples:
>>
>> void mapProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(() ->
>> text.getValueSafe().toUpperCase(), text));
>>
>> // Fluent: much more compact, no need to handle null
>> label.textProperty().bind(text.map(String::toUpperCase));
>> }
>>
>> void calculateCharactersLeft() {
>> // Standard JavaFX:
>>
>> label.textProperty().bind(text.length().negate().add(100).asString().concat("
>> characters left"));
>>
>> // Fluent: slightly more compact and more clear (no negate needed)
>> label.textProperty().bind(text.orElse("").map(v -> 100 - v.length() +
>> " characters left"));
>> }
>>
>> void mapNestedValue() {
>> // Standard JavaFX:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> employee.get() == null ? ""
>> : employee.get().getCompany() == null ? ""
>> : employee.get().getCompany().getName(),
>> employee
>> ));
>>
>> // Standard JavaFX + Optional:
>> label.textProperty().bind(Bindings.createStringBinding(
>> () -> Optinal.ofNullable(employee.get())
>> .map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse(""),
>> employee
>> ));
>>
>> // Fluent: no need to handle nulls everywhere
>> label.textProperty().bind(
>> employee.map(Employee::getCompany)
>> .map(Company::getName)
>> .orElse("")
>> );
>> }
>>
>> void mapNestedProperty() {
>> // Standard JavaFX:
>> label.textProperty().bind(
>> Bindings.when(Bindings.selectBoolean(label.sceneProperty(),
>> "window", "showing"))
>> .then("Visible")
>> .otherwise("Not Visible")
>> );
>>
>> // Fluent: type safe
>> label.textProperty().bind(label.sceneProperty()
>> .flatMap(Scene::windowProperty)
>> .flatMap(Window::showingProperty)
>> .orElse(false)
>> .map(showing -> showing ? "Visible" : "Not Visible")
>> );
>> }
>>
>> Note that this is based on ideas in ReactFX and my own experiments in
>> https://github.com/hjohn/hs.jfx.eventstream. I've come to the conclusion
>> that this is much better directly integrated into JavaFX, and I'm hoping
>> this proof of concept will be able to move such an effort forward.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix wording
I reviewed the public API changes, and this looks like a great addition to
JavaFX bindings. I think there might be time to get this into JavaFX 19,
presuming that there are no issues with the testing or implementation, so let's
proceed down that path.
I left one comment on the API docs as well as pointed out the public methods
that will need an `@since 19` javadoc tag.
Once that is updated you can propagate the javadoc changes to the CSR
(including the `@since` tags) and move it to "Proposed". I'll formally review
it later, once the code review is closer to being done.
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
On Fri, 27 May 2022 16:47:04 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Adingef else block to unregisterListener Looks good. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v9]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Adingef else block to unregisterListener - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/79f30067..c609ad9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=07-08 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
On Fri, 27 May 2022 15:42:09 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Change for unregisterDomWindow function and code cleanup > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 57: > >> 55: } >> 56: >> 57: if (it->second && it->second->use_count() > 1) > > Shouldn't this be `else if`? The previous block calls `erase(it)`, so it > isn't valid to dereference `it` after that call. Applied and tested - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
On Fri, 27 May 2022 12:13:43 GMT, Jay Bhaskar wrote: >> This PR is new implementation of JavaEvent listener memory management. >> Issue >> [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) >> >> 1. Calling remove event listener does not free jni global references. >> 2. When WebView goes out of scope (disposed from app) , its Event Listeners >> are not being garbage collected. >> >> Solution: >> 1. Detached the jni global reference from JavaEventListener. >> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni >> global reference. >> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. >> 4. EventListenerManager is a singleton class , which stores the >> JavaObjectWrapperHandler mapped with JavaEventListener. >> 5. EventListenerManager also stores the JavaEventListener mapped with >> DOMWindow. >> 6. When Event listener explicitly removed , JavaEventListener is being >> forwarded to EventListenerManager to clear the listener. >> 7. When WebView goes out of scope, EventListenerManager will de-registered >> all the event listeners based on the ref counts attached with WebView >> DOMWindow. > > Jay Bhaskar has updated the pull request incrementally with one additional > commit since the last revision: > > Change for unregisterDomWindow function and code cleanup The changes look good. While doing my final review I noticed one more thing that I think should be changed. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 57: > 55: } > 56: > 57: if (it->second && it->second->use_count() > 1) Shouldn't this be `else if`? The previous block calls `erase(it)`, so it isn't valid to dereference `it` after that call. - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v7]
On Thu, 26 May 2022 17:00:46 GMT, Kevin Rushforth wrote: >> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding space for map include > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 106: > >> 104: else >> 105: isReferringToOtherListener = true; >> 106: } > > I think I now understand what this is trying to do, but it doesn't looks like > it's implemented correctly nor is it optimal. > > It seems that the `isReferringToOtherListener` flag is intended to be true > iff there is any `JavaEventListener` with a ref count > 1 associated with the > Window being unregistered. Ignoring any possible bugs in how it is > implemented, there are two problems with this approach. First, you want to > remove entries associated with a particular listener if _that_ listener isn't > used by another window. So a global "is any listener referring to another > window" isn't what you want. Second, since this is multimap, it would seem > better to remove individual `(key,value)` pairs associated with the specific > window being removed rather than calling erase only for those listeners that > aren't being shared at all. If this is feasible, then you wouldn't have to > even care if that listener is used by a node in another window. I'm not > familiar enough with C++ multimap calls to know how easy this would be, but > presumably, it shouldn't be too hard. > > Not directly related to this, it might be cleaner to move this logic to > `unregisterDOMWindow` instead of having a separate `resetDOMWindow` method, > since this is really part of the same conceptual operation. I have used the suggested method , and tested it works expected. Thanks - PR: https://git.openjdk.java.net/jfx/pull/799
Re: RFR: 8088420: JavaFX WebView memory leak via EventListener [v8]
> This PR is new implementation of JavaEvent listener memory management. > Issue > [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1) > > 1. Calling remove event listener does not free jni global references. > 2. When WebView goes out of scope (disposed from app) , its Event Listeners > are not being garbage collected. > > Solution: > 1. Detached the jni global reference from JavaEventListener. > 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni > global reference. > 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener. > 4. EventListenerManager is a singleton class , which stores the > JavaObjectWrapperHandler mapped with JavaEventListener. > 5. EventListenerManager also stores the JavaEventListener mapped with > DOMWindow. > 6. When Event listener explicitly removed , JavaEventListener is being > forwarded to EventListenerManager to clear the listener. > 7. When WebView goes out of scope, EventListenerManager will de-registered > all the event listeners based on the ref counts attached with WebView > DOMWindow. Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision: Change for unregisterDomWindow function and code cleanup - Changes: - all: https://git.openjdk.java.net/jfx/pull/799/files - new: https://git.openjdk.java.net/jfx/pull/799/files/d6fd438b..79f30067 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=799=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=799=06-07 Stats: 74 lines in 5 files changed: 11 ins; 23 del; 40 mod Patch: https://git.openjdk.java.net/jfx/pull/799.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799 PR: https://git.openjdk.java.net/jfx/pull/799
