On Fri, 17 Apr 2020 07:22:20 GMT, dannygonzalez <github.com+6702882+dannygonza...@openjdk.org> wrote:
>> I've tested this pull request locally a few times, and the performance >> improvement is quite significant. A test with 20.000 nested stack panes >> resulted in these average times: >> >> - Add all 51 ms >> - Remove all 25 ms >> >> Versus the unmodified code: >> >> - Add all 34 ms >> - Remove all 135 ms >> >> However, there are some significant functional changes as well that might >> impact current users: >> >> 1. The new code ensures that all listeners are notified even if the list is >> modified during iteration by always making a **copy** when an event is >> fired. The old code only did so when it was **actually** modified during >> iteration. This can be mitigated by making the copy in the code that >> modifies the list (as the original did) using the `locked` flag to check >> whether an iteration was in progress. >> >> 2. There is a significant increase in memory use. Where before each >> listener occupied an entry in an array, now each listener is wrapped by >> `Map.Entry` (the Integer instance used per entry can be disregarded). I >> estimate around 4-8x more heap will be consumed (the numbers are small >> however, still well below 1 MB for 20.000 entries). If this is an issue, a >> further level could be introduced in the listener implementation hierarchy >> (singleton -> array -> map). >> >> 3. Even though the current version of this pull request takes care to notify >> duplicate listeners the correct amount of times, it does not notify them in >> the correct order with respect to other listeners. If one registers >> listeners (a, b, a) the notification order will be (a, a, b). >> >> The last point is not easily solved and could potentially cause problems. >> >> Finally I think this solution, although it performs well is not the full >> solution. A doubling or quadrupling of nodes would again run into serious >> limitations. I think this commit >> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd >> should not have introduced another listener for each Node on the Window >> class. A better solution would be to only have the Scene listen to Window >> and have Scene provide a new combined status property that Node could use >> for its purposes. >> >> Even better however would be to change the properties involved to make use >> of the hierarchy naturally present in Nodes, having child nodes listen to >> their parent, and the top level node listen to the scene. This would reduce >> the amount of listeners on a single property in Scene and Window immensely, >> instead spreading those listeners over the Node hierarchy, keeping listener >> lists much shorter, which should scale a lot better. > > @hjohn > >> 2. There is a significant increase in memory use. Where before each listener >> occupied an entry in an array, now each listener is wrapped by Map.Entry >> (the Integer instance used per entry can be disregarded). I estimate around >> 4-8x more heap will be consumed (the numbers are small however, still well >> below 1 MB for 20.000 entries). If this is an issue, a further level could >> be introduced in the listener implementation hierarchy (singleton -> array >> -> map). > > There was discussion about lazy initialisation of the LinkedHashMap when > needed and/or have some sort of strategy where we could use arrays or lists > if the number of listeners are less than some threshold (i.e. introducing > another level to the hierarchy as you mentioned). > This was mentioned here also: > https://github.com/openjdk/jfx/pull/108#issuecomment-590838942 I've implemented an alternative solution: Removing the listeners on `Window.showingProperty` and `Scene.windowProperty` completely. They are in fact only used in two places: `PopupWindow` in order to remove itself if the `Window` is no longer showing, and `ProgressIndicatorSkin`. These two can be easily replaced with their own listeners for these properties instead of burdening **all** nodes with these listeners only to support these two classes. I left the `isTreeShowing` method in, and implemented it simply as `isTreeVisible() && isWindowShowing()` as that's really the only difference between "visible" and "showing" apparently. Here is the test result with 20.000 nested StackPanes with only this change in: - Add all 45 ms - Remove all 25 ms I think this might be a good solution as it completely avoids these listeners. @dannygonzalez I added a proof of concept here if you want to play with it: https://github.com/openjdk/jfx/pull/185 ------------- PR: https://git.openjdk.java.net/jfx/pull/108
