On Fri, 17 Apr 2020 07:22:20 GMT, dannygonzalez <github.com+6702882+dannygonza...@openjdk.org> wrote:
>> @hjon >> >>> 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). >> >> Unless I'm missing something I don't think this is the case. I used a >> LinkedHashMap which preserved the order of >> notifications. Actually some unit tests failed if the notifications weren't >> carried out in the same order as >> registration which was the case when I used a HashMap. See here: >> https://github.com/openjdk/jfx/pull/108#issuecomment-590883183 > > @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. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
