Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic [v2]

[dannygonzalez]

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

dannygonzalez has updated the pull request incrementally.

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/108/files
  - new: https://git.openjdk.java.net/jfx/pull/108/files/05c37196..05c37196

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=108=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=108=00-01

  Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/108.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/108/head:pull/108

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Wed, 26 Feb 2020 09:49:04 GMT, Jeanette Winzenburg  
wrote:

> Hmm .. personally, I would consider changing such a basic (and probably 
> highly optimized) implementation used all over the framework is a very high 
> risk change that at the worst outcome would detoriate internal and external 
> code. So before going that lane, I would try - as you probably have, just me 
> not seeing it :) - to tackle the problem from the other end:
> 
> > I know that in our application, the **thousands of listeners** 
> > unregistering when using a TableView was stalling the JavaFX thread.
> 
> .. sounds like a lot. Any idea, where exactly they come into play?

I did start to look at why there were so many change listeners unregistering 
but felt that would take a deeper understanding of the architecture and design 
decisions of JavaFX scene graph and I didn't have that time to invest. 
I do know that there are thousands of them unregistering in a TableView and 
unregistering them is a bottleneck for the JavaFX thread.

There are multiple change listeners on a Node for example, so you can imagine 
with the number of nodes in a TableView this is going to be a problem if the 
unregistering is slow.

To get our application usable I profiled the code and saw this unregistering as 
a major bottleneck, hence why I looked at this more obvious solution.

I'm happy to look at the problem from the other angle and happy to listen to 
suggestions from people with more experience of the design and architecture but 
tackling the problem from the perspective of re-architecting the behaviour of 
listeners in the scene graph would be more work than I could feasibly take on.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 25 Feb 2020 00:45:06 GMT, Kevin Rushforth  wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 194:
>> 
>>> 192: 
>>> 193: private Map 
>>> invalidationListeners = new LinkedHashMap<>();
>>> 194: private Map, Integer> 
>>> changeListeners = new LinkedHashMap<>();
>> 
>> Two comments on this:
>> 
>> 1. The old implementation initialized these lazily, so we might want to 
>> preserve that behavior. I think it is reasonable since in most cases an 
>> observable won't have both types of listeners attached to it.
>> 2. Since we're dealing wither performance optimizations here, we should 
>> think about what the `initialCapcity` and `loadFactor` of these maps should 
>> be, as it can greatly increase performance when dealing with a large amount 
>> of entries.
>
> Adding to this, we need to ensure that the simple case of a few listeners 
> doesn't suffer memory bloat. It may make sense to initially allocate a Map 
> with a small capacity and load factor, and then reallocate it if someone adds 
> more than a certain number of listeners.

Agree with the lazy initialisation and the initial setting of capacity and load 
factor to reduce memory footprint unless it's needed.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Mon, 24 Feb 2020 10:16:55 GMT, dannygonzalez 
 wrote:

>>> So we need to keep the original behaviour from before where a count was 
>>> checked on every addListener call and the weak references were purged from 
>>> the array using the original calculation for this count.
>> 
>> The GC'd weak listeners do need to be purged, but why is the original 
>> behavior of the counters still applicable?
>
> Just wanted to keep a similar behaviour as before using the same calculation 
> based originally on the size of the listeners array list and now based on the 
> size of the map. So in theory the weak listeners should be trimmed at the 
> same rate as before.
> Happy to hear alternatives.

Also bear in mind that the trimming of weak listeners is extremely slow as it 
has to iterate through each listener performing the weak listener test. We 
can't call this every time we add a listener as this would lock up the JavaFX 
thread completely (I tried it). 
I assume this is why the original calculation was used where it backs of the 
rate the weak listener trimming code was called as the array list grew.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Sun, 23 Feb 2020 01:05:25 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 283:
> 
>> 281: // while the observers are being notified is safe
>> 282: final Map 
>> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
>> 283: final Map, Integer> curChangeList 
>> = new LinkedHashMap<>(changeListeners);
> 
> You only need the entry set, so you don't need to copy the map, just the set.

Thanks, yes the EntrySet would make more sense here. I'll fix that up.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 285:
> 
>> 283: final Map, Integer> curChangeList 
>> = new LinkedHashMap<>(changeListeners);
>> 284: 
>> 285: curInvalidationList.entrySet().forEach(entry -> 
>> fireInvalidationListeners(entry));
> 
> The lambda can be converted to a method reference.

Agreed,  I'll fix.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>  line 64:
> 
>> 62: ((WeakListener)t).wasGarbageCollected();
>> 63: 
>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
> 
> This can be `listeners.keySet().removeIf(p::test);`.

Agreed, will change.

> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 197:
> 
>> 195: private T currentValue;
>> 196: private int weakChangeListenerGcCount = 2;
>> 197: private int weakInvalidationListenerGcCount = 2;
> 
> Why are these set to 2 and why do you need them at all? The previous 
> implementation needed to grow and shrink the array so it had to keep these, 
> but `Map` takes care of this for you.

I agree, I kept these in as I wasn't sure if there was a need to manually force 
the garbage collection of weak listeners at the same rate as the original 
implementation.
Removing this would make sense to me also.

Updated my thoughts on this, see my comments below.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Mon, 24 Feb 2020 08:14:02 GMT, dannygonzalez 
 wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 197:
>> 
>>> 195: private T currentValue;
>>> 196: private int weakChangeListenerGcCount = 2;
>>> 197: private int weakInvalidationListenerGcCount = 2;
>> 
>> Why are these set to 2 and why do you need them at all? The previous 
>> implementation needed to grow and shrink the array so it had to keep these, 
>> but `Map` takes care of this for you.
>
> I agree, I kept these in as I wasn't sure if there was a need to manually 
> force the garbage collection of weak listeners at the same rate as the 
> original implementation.
> Removing this would make sense to me also.
> 
> Updated my thoughts on this, see my comments below.

As far as I know a LinkedHashMap doesn't automatically remove weak listener 
entries. The listener maps can be holding weak listeners as well as normal 
listeners.
So we need to keep the original behaviour from before where a count was checked 
on every addListener call and the weak references were purged from the array 
using the original calculation for this count.
Otherwise the map would never garbage collect these weak references.

The initial value of 2 for these counts was just the starting count although 
this is not necessarily strictly accurate. To be completely accurate then we 
would have to set the appropriate count in each constructor as follows:

i.e. in the Constructor with 2 InvalidationListeners:
weakChangeListenerGcCount = 0
weakInvalidationListenerGcCount = 2

in the Constructor with 2 ChangeListeners:
weakChangeListenerGcCount = 2
weakInvalidationListenerGcCount = 0

in the Constructor with 1 InvalidationListener and 1 ChangeListener:
weakChangeListenerGcCount = 1
weakInvalidationListenerGcCount = 1

Now, I could have used a WeakHashMap to store the listeners where it would 
automatically purge weak listeners but this doesn't maintain insertion order. 
Even though the specification doesn't mandate that listeners should be called 
back in the order they are registered, the unit tests failed when I didn't 
maintain order.

I am happy to remove this weak listener purge code (as it would make the code 
much simpler) but then we wouldn't automatically remove the weak listeners, but 
this may not be deemed a problem anyway?

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>>  line 64:
>> 
>>> 62: ((WeakListener)t).wasGarbageCollected();
>>> 63: 
>>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>> 
>> This can be `listeners.keySet().removeIf(p::test);`.
>
> Agreed, will change.

Nope, actually:
`listeners.keySet().removeIf(p::test) `

is not the same as:
`listeners.entrySet().removeIf(e -> p.test(e.getKey()));`

We need to test against the entrySet.key not the entrySet itself.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Mon, 24 Feb 2020 09:59:28 GMT, Nir Lisker  wrote:

>> As far as I know a LinkedHashMap doesn't automatically remove weak listener 
>> entries. The listener maps can be holding weak listeners as well as normal 
>> listeners.
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
>> Otherwise the map would never garbage collect these weak references.
>> 
>> The initial value of 2 for these counts was just the starting count although 
>> this is not necessarily strictly accurate. To be completely accurate then we 
>> would have to set the appropriate count in each constructor as follows:
>> 
>> i.e. in the Constructor with 2 InvalidationListeners:
>> weakChangeListenerGcCount = 0
>> weakInvalidationListenerGcCount = 2
>> 
>> in the Constructor with 2 ChangeListeners:
>> weakChangeListenerGcCount = 2
>> weakInvalidationListenerGcCount = 0
>> 
>> in the Constructor with 1 InvalidationListener and 1 ChangeListener:
>> weakChangeListenerGcCount = 1
>> weakInvalidationListenerGcCount = 1
>> 
>> Now, I could have used a WeakHashMap to store the listeners where it would 
>> automatically purge weak listeners but this doesn't maintain insertion 
>> order. Even though the specification doesn't mandate that listeners should 
>> be called back in the order they are registered, the unit tests failed when 
>> I didn't maintain order.
>> 
>> I am happy to remove this weak listener purge code (as it would make the 
>> code much simpler) but then we wouldn't automatically remove the weak 
>> listeners, but this may not be deemed a problem anyway?
>
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
> 
> The GC'd weak listeners do need to be purged, but why is the original 
> behavior of the counters still applicable?

Just wanted to keep a similar behaviour as before using the same calculation 
based originally on the size of the listeners array list and now based on the 
size of the map. So in theory the weak listeners should be trimmed at the same 
rate as before.
Happy to hear alternatives.

>> Nope, actually:
>> `listeners.keySet().removeIf(p::test) `
>> 
>> is not the same as:
>> `listeners.entrySet().removeIf(e -> p.test(e.getKey()));`
>> 
>> We need to test against the entrySet.key not the entrySet itself.
>
>> We need to test against the entrySet.key not the entrySet itself.
> 
> I suggested to test against the elements in `keySet()`, which are the same as 
> the ones in `entrySet().getKey()`.

Gotcha, sorry I missed that.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 8 Sep 2020 20:14:48 GMT, Kevin Rushforth  wrote:

>> I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM 
>> and look at the hotspots in the JavaFX thread, you can see that about 45% of 
>> the time in the JavaFX thread is spent in removeListener calls.
>> 
>> Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
>> profile packages section".
>> 
>> [JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)
>
> @dannygonzalez Per [this 
> message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html)
>  on the openjfx-dev mailing list, I have filed a new JBS issue for this PR to 
> use. Please change the title to:
> 
> 8252936: Optimize removal of listeners from ExpressionHelper.Generic

Thanks @kevinrushforth, I've changed the title.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Fri, 17 Apr 2020 10:46:51 GMT, dannygonzalez 
 wrote:

>> The problem is that there are usually many nodes, but only very few scenes 
>> and windows, so registering a listener for each node on a scene or window is 
>> pretty bad design (also consider the amount of notifications that a scene 
>> change would trigger in such scenarios).  As far as I can see, this is the 
>> only such listener and only needed for two very limited cases, and its 
>> addition in the past may have slipped through the cracks.
>> 
>> Adding a performance unit test that specifically checks add/remove 
>> performance of nodes may prevent such future regressions.
>
> @hjohn, agreed regards the issues of adding a listener to each node.
> 
> Would it be worth doing the additional work of updating PopupWindow and 
> ProgressIndicatorSkin to add their own listeners to make this a pull request 
> that can be reviewed officially?
> 
> I await any further comments from @kevinrushforth et al.

I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM and 
look at the hotspots in the JavaFX thread, you can see that about 45% of the 
time in the JavaFX thread is spent in removeListener calls.

Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
profile packages section".

[JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Fri, 17 Apr 2020 10:22:15 GMT, John Hendrikx  wrote:

>> @hjohn Thanks for looking into this. It looks like your changes do improve 
>> the issue with the JavaFX thread being swamped with listener 
>> de-registrations. Looking at the JavaFX thread in VisualVM, the 
>> removeListener call has dropped off the hotspots in the same way it did with 
>> my pull request.
>> 
>> I wasn't fully confident of making changes to the Node hierarchy to remove 
>> listeners hence why I approached the issue from the other direction i.e. the 
>> obvious bottleneck which was the listener de-registration slowness.
>> 
>> I do worry however that any changes down the road which add listeners to the 
>> Node hierarchy again without fully understanding the implications would lead 
>> us to the same point we are now where the slowness of listener 
>> de-registrations becomes an issue again. There are no tests that catch this 
>> scenario. 
>> I feel that ideally both solutions are needed but am happy to bow to the 
>> more experienced OpenJFX devs opinions here as I know my changes may be more 
>> fundamental and hence risky.
>
> The problem is that there are usually many nodes, but only very few scenes 
> and windows, so registering a listener for each node on a scene or window is 
> pretty bad design (also consider the amount of notifications that a scene 
> change would trigger in such scenarios).  As far as I can see, this is the 
> only such listener and only needed for two very limited cases, and its 
> addition in the past may have slipped through the cracks.
> 
> Adding a performance unit test that specifically checks add/remove 
> performance of nodes may prevent such future regressions.

@hjohn, agreed regards the issues of adding a listener to each node.

Would it be worth doing the additional work of updating PopupWindow and 
ProgressIndicatorSkin to add their own listeners to make this a pull request 
that can be reviewed officially?

I await any further comments from @kevinrushforth et al.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Fri, 17 Apr 2020 08:07:08 GMT, John Hendrikx  wrote:

>> @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
>
> @dannygonzalez I added a proof of concept here if you want to play with it: 
> https://github.com/openjdk/jfx/pull/185

@hjohn Thanks for looking into this. It looks like your changes do improve the 
issue with the JavaFX thread being swamped with listener de-registrations. 
Looking at the JavaFX thread in VisualVM, the removeListener call has dropped 
off the hotspots in the same way it did with my pull request.

I wasn't fully confident of making changes to the Node hierarchy to remove 
listeners hence why I approached the issue from the other direction i.e. the 
obvious bottleneck which was the listener de-registration slowness.

I do worry however that any changes down the road which add listeners to the 
Node hierarchy again without fully understanding the implications would lead us 
to the same point we are now where the slowness of listener de-registrations 
becomes an issue again. There are no tests that catch this scenario. 
I feel that ideally both solutions are needed but am happy to bow to the more 
experienced OpenJFX devs opinions here as I know my changes may be more 
fundamental and hence risky.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Thu, 16 Apr 2020 16:15:20 GMT, John Hendrikx  wrote:

>> @hjohn I have 12136 change listeners when debugging our application as you 
>> suggested.
>> 
>> Please note that I see the issue when the TableView is having items added to 
>> it. If you just have a static TableView I do not see the issue.
>> 
>> It is only when you add items to the TableView which causes a myriad of 
>> listeners to be deregistered and registered.
>> The Visual VM snapshot I attached above was taken as our application was 
>> adding items to the TableView.
>
> 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.

@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

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Thu, 16 Apr 2020 08:46:26 GMT, John Hendrikx  wrote:

>> If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
>> ExpressionHelper.removeListener is using 61% of the JavaFX thread whilst 
>> running our application.
>> 
>> [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)
>> 
>> If you show only the JavaFX Application thread, press the "HotSpot" and 
>> "Reverse Calls" button you can take a look to see which classes are calling 
>> the removeListener function.
>> 
>> ![Screenshot 2020-04-16 at 09 16 
>> 11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)
>
> @dannygonzalez  Could you perhaps debug your application and take a look at 
> how large the following array is: a random node -> `scene` -> `value` -> 
> `window` -> `readOnlyProperty` -> `helper` -> `changeListeners`.  I just 
> tested this with a custom control displaying 200 cells on screen at once 
> (each cell consisting of about 30 nodes itself), and I saw about 2 change 
> listeners registered on this single Scene Window property.
> 
> However, this custom control is not creating/destroying cells beyond the 
> initial allocation, so there wouldn't be any registering and unregistering 
> going on, scrolling was still smooth >30 fps.

@hjohn I have 12136 change listeners when debugging our application as you 
suggested.

Please note that I see the issue when the TableView is having items added to 
it. If you just have a static TableView I do not see the issue.

It is only when you add items to the TableView which causes a myriad of 
listeners to be deregistered and registered.
The Visual VM snapshot I attached above was taken as our application was adding 
items to the TableView.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Thu, 16 Apr 2020 07:34:40 GMT, John Hendrikx  wrote:

>> Looking at the commit  
>> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>>  
>>  it seems that the long listener lists are actually part of the `Scene`'s 
>> `Window` property and the `Window`'s `Showing` property.  Each `Node` 
>> registers itself on those and so the listener lists for those properties 
>> would scale with the number of nodes.
>> 
>> A test case showing this problem would really be great as then the patch can 
>> also be verified to solve the problem, but I suppose it could be reproduced 
>> simply by having a large number of Nodes in a scene.  @dannygonzalez could 
>> you give us an idea how many Nodes we're talking about?  1000? 10.000?
>> 
>> It also means there might be other options, do Nodes really need to add 
>> these listeners and for which functionality and are there alternatives?  It 
>> would also be possible to target only these specific properties with an 
>> optimized listener list to reduce the impact of this change.
>
> The listeners added by `Node` are apparently internally required for internal 
> properties `TreeShowing` and `TreeVisible`, and are used to take various 
> decisions like whether to play/pause animations.  There is also a couple of 
> listeners registering on these properties in turn (in `PopupWindow`, 
> `SwingNode`, `WebView` and `MediaView`).
> 
> A lot of the checks for visibility / showing could easily be done by using 
> the `Scene` property and checking visibility / showing status from there.  No 
> need for so many listeners.  The other classes mentioned might register their 
> own listener, instead of having `Node` do it for them (and thus impacting 
> *every* node).
> 
> Alternatively, `Node` may lazily register the listeners for Scene.Window and 
> Window.Showing only when needed (which from what I can see is for pretty 
> specific classes only, not classes that you'd see a lot in a TableView...)

If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
ExpressionHelper.removeListener is using 61% of the JavaFX thread whilst 
running our application.

[snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)

If you show only the JavaFX Application thread, press the "HotSpot" and 
"Reverse Calls" button you can take a look to see which classes are calling the 
removeListener function.

![Screenshot 2020-04-16 at 09 16 
11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 14 Apr 2020 12:20:10 GMT, John Hendrikx  wrote:

>>> Hmm .. personally, I would consider changing such a basic (and probably 
>>> highly optimized) implementation used all over the framework is a very high 
>>> risk change that at the worst outcome would detoriate internal and external 
>>> code. So before going that lane, I would try - as you probably have, just 
>>> me not seeing it :) - to tackle the problem from the other end:
>>> 
>>> > I know that in our application, the **thousands of listeners** 
>>> > unregistering when using a TableView was stalling the JavaFX thread.
>>> 
>>> .. sounds like a lot. Any idea, where exactly they come into play?
>> 
>> I did start to look at why there were so many change listeners unregistering 
>> but felt that would take a deeper understanding of the architecture and 
>> design decisions of JavaFX scene graph and I didn't have that time to 
>> invest. 
>> I do know that there are thousands of them unregistering in a TableView and 
>> unregistering them is a bottleneck for the JavaFX thread.
>> 
>> There are multiple change listeners on a Node for example, so you can 
>> imagine with the number of nodes in a TableView this is going to be a 
>> problem if the unregistering is slow.
>> 
>> To get our application usable I profiled the code and saw this unregistering 
>> as a major bottleneck, hence why I looked at this more obvious solution.
>> 
>> I'm happy to look at the problem from the other angle and happy to listen to 
>> suggestions from people with more experience of the design and architecture 
>> but tackling the problem from the perspective of re-architecting the 
>> behaviour of listeners in the scene graph would be more work than I could 
>> feasibly take on.
>
> @dannygonzalez You mentioned "There are multiple change listeners on a Node 
> for example, so you can imagine with the number of nodes in a TableView this 
> is going to be a problem if the unregistering is slow.".
> 
> Your changes in this PR seem to be focused on improving performance of 
> unregistering listeners when there are thousands of listeners listening on 
> changes or invalidations of the **same** property.  Is this actually the 
> case?  Is there a single property in TableView or TreeTableView where such a 
> high amount of listeners are present?  If so, I think the problem should be 
> solved there.
> 
> As it is, I donot think this PR will help unregistration performance of 
> listeners when the listeners are  spread out over dozens of properties, and 
> although high in total number, the total listeners per property would be low 
> and their listener lists would be short.  Performance of short lists (<50 
> entries) is almost unbeatable, so it is relevant for this PR to know if there 
> are any properties with long listener lists.

@hjohn 
I haven't quantified exactly where all the listeners are coming from but the 
Node class for example has various listeners on it.

The following changeset  (which added an extra listener on the Node class) 
impacted TableView performance significantly (although it was still bad before 
this change in my previous tests):

> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
> Author: Florian Kirmaier mailto:fk at sandec.de>>
> Date:   Tue Jan 22 09:08:36 2019 -0800
> 
> 8216377: Fix memoryleak for initial nodes of Window
> 8207837: Indeterminate ProgressBar does not animate if content is added 
> after scene is set on window
> 
> Add or remove the windowShowingChangedListener when the scene changes

As you can imagine, a TableView with many columns and rows can be made up of 
many Node classes. The impact of having multiple listeners deregistering on the 
Node when new rows are being added to a TableView can be a significant 
performance hit on the JavaFX thread. 

I don't have the time or knowledge to investigate why these listeners are all 
needed especially around the Node class. I went directly to the source of the 
problem which was the linear search to deregister each listener.

I have been running my proposed fix in our JavaFX application for the past few 
months and it has saved it from being unusable due to the JavaFx thread being 
swamped.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Mon, 17 Feb 2020 13:51:49 GMT, Kevin Rushforth  wrote:

>> The listeners are called back in the order they were registered in my 
>> implementation although I didn’t see this requirement in the spec unless I 
>> missed something.
>> 
>> The performance unregistering thousands of listeners by TableView from the 
>> array is killing the performance of our JavaFX application. It takes up to 
>> 60% of JavaFX Application thread CPU and there are various bug reports 
>> around this same TableView performance issue.
>> The set implementation has lowered this to below 1%.
>> 
>> This pull request may be too fundamental a change to go into mainline. We 
>> however have to build our local OpenJFX with this fix or our application is 
>> unusable.
>> 
>> I’m happy to receive any suggestions.
>> 
>> Danny
>> 
>> 
>> On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
>> mailto:notificati...@github.com>> wrote:
>> 
>> 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
>> whatever drove it (had been so since the beginning ot java desktop, at least 
>> since the days of swing), there is no way to change it, is it?
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
>> yeah, the test coverage is ... not optimal :)
>> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
>> 
>> a count plus some marker as to where it was added:
>> 
>> addListener(firstL)
>> addListener(secondL)
>> addListener(firstL)
>> 
>> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
>> which brings us back to .. an array?
>> 
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on 
>> GitHub<https://github.com/openjdk/jfx/pull/108?email_source=notifications_token=ABTEOIWBBLX3XA3JV23OP6LRCPSXRA5CNFSM4KQ7YBNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELQSLEY#issuecomment-585180563>,
>>  or 
>> unsubscribe<https://github.com/notifications/unsubscribe-auth/ABTEOIVSPJEJ6FAJ5SFT7V3RCPSXRANCNFSM4KQ7YBNA>.
>
> @dannygonzalez the reason for the `jcheck` failure is that you have commits 
> with two different email addresses in your branch. At this point, it's 
> probably best to squash the commits into a single commit with `git rebase -i 
> master` (presuming that your local `master` is up to date), and then do a 
> force-push.

@kevinrushforth just a note to say there are other ExpressionHelper classes 
(i.e. MapExpressionHelper, SetExpressionHelper and ListExpressionHelper) that 
also use arrays and suffer from the linear search issue when removing 
listeners. 

These however didn't appear in the critical path of the JavaFXThread and didn't 
come up in my profiling of TableView.

If this pull request is accepted, my opinion is that they should probably all 
move to using the same pattern as here, which is to use Maps instead of Arrays 
for their listener lists so that all these classes are uniform.

Thanks

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 25 Feb 2020 13:55:50 GMT, Nir Lisker  wrote:

>> Replying to @nlisker and @kevinrushforth general comments about the memory 
>> consumption of using a LinkedHashMap:
>> 
>> I agree that at the very least these should be lazy initialised when they 
>> are needed and that we should initially allocate a small capacity and load 
>> factor.
>> 
>> We could also have some sort of strategy where we could use arrays or lists 
>> if the number of listeners are less than some threshold (i.e. keeping the 
>> implementation with a similar overhead to the original) and then switch to 
>> the HashMap when it exceeds this threshold. This would make the code more 
>> complicated and I wonder if this is the worth the extra complexity.
>>  
>> I know that in our application, the thousands of listeners unregistering 
>> when using a TableView was stalling the JavaFX thread. 
>> 
>> I am happy to submit code that lazy initialises and minimises initial 
>> capacity and load factor as suggested or happy to take direction and 
>> suggestions from anyone with a deeper understanding of the code than myself.
>
> I think that a starting point would be to decide on a spec for the listener 
> notification order: is it specified by their registration order, or that is 
> it unspecified, in which case we can take advantage of this for better 
> performance. Leaving is unspecified and restricting ourselves to having it 
> ordered is losing on both sides.

Even though the order of notifications is unspecified, the following tests fail 
if we use a HashMap vs LinkedHashMap i.e. the notifications are not in order of 
registration:

test.javafx.scene.control.TableViewTest > 
ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:470)
at org.junit.Assert.assertEquals(Assert.java:454)
at 
test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)

test.javafx.scene.control.TreeTableViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)

test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected: but 
was:
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:145)
at 
test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)


These would need to be investigated to see why they assume this order.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 25 Feb 2020 00:43:34 GMT, Kevin Rushforth  wrote:

>> Sorry for the interruption, send a PR that corrects the same problem.
>
> I haven't done a detailed review yet, but I worry about the memory 
> consumption and performance of using a Map for the simple cases where there 
> is only a single (or very small number) of listeners. These methods are used 
> in many more places than just the TableView / TreeTableView implementation.

Replying to @nlisker and @kevinrushforth general comments about the memory 
consumption of using a LinkedHashMap:

I agree that at the very least these should be lazy initialised when they are 
needed and that we should initially allocate a small capacity and load factor.

We could also have some sort of strategy where we could use arrays or lists if 
the number of listeners are less than some threshold (i.e. keeping the 
implementation with a similar overhead to the original) and then switch to the 
HashMap when it exceeds this threshold. This would make the code more 
complicated and I wonder if this is the worth the extra complexity.
 
I know that in our application, the thousands of listeners unregistering when 
using a TableView was stalling the JavaFX thread. 

I am happy to submit code that lazy initialises and minimises initial capacity 
and load factor as suggested or happy to take direction and suggestions from 
anyone with a deeper understanding of the code than myself.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Wed, 12 Feb 2020 12:22:15 GMT, Jeanette Winzenburg  
wrote:

>>> hmm ... wouldn't the change violate spec of adding listeners:
>>> 
>>> > If the same listener is added more than once, then it will be notified 
>>> > more than once.
>> 
>> True, I hadn't read that spec in ObservableValueBase. 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
>
>> 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
> 
> whatever drove it (had been so since the beginning ot java desktop, at least 
> since the days of swing), there is no way to change it, is it?
> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
> 
> yeah, the test coverage is ... not optimal :)
> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
> 
> a count plus some marker as to where it was added: 
> 
>addListener(firstL)
>addListener(secondL)
>addListener(firstL)
> 
> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
> which brings us back to .. an array?

The listeners are called back in the order they were registered in my 
implementation although I didn’t see this requirement in the spec unless I 
missed something.

The performance unregistering thousands of listeners by TableView from the 
array is killing the performance of our JavaFX application. It takes up to 60% 
of JavaFX Application thread CPU and there are various bug reports around this 
same TableView performance issue.
The set implementation has lowered this to below 1%.

This pull request may be too fundamental a change to go into mainline. We 
however have to build our local OpenJFX with this fix or our application is 
unusable.

I’m happy to receive any suggestions.

Danny


On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
mailto:notificati...@github.com>> wrote:


Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

whatever drove it (had been so since the beginning ot java desktop, at least 
since the days of swing), there is no way to change it, is it?

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

yeah, the test coverage is ... not optimal :)

I would need to store a registration count for each listener to satisfy this 
requirement.

a count plus some marker as to where it was added:

addListener(firstL)
addListener(secondL)
addListener(firstL)

must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
which brings us back to .. an array?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

I have signed the Oracle Contributor Agreement today. Have not received back 
any confirmation yet though.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Wed, 12 Feb 2020 11:29:24 GMT, Jeanette Winzenburg  
wrote:

> hmm ... wouldn't the change violate spec of adding listeners:
> 
> > If the same listener is added more than once, then it will be notified more 
> > than once.

True, I hadn't read that spec in ObservableValueBase. 
Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

I would need to store a registration count for each listener to satisfy this 
requirement.

-

PR: https://git.openjdk.java.net/jfx/pull/108

RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

https://bugs.openjdk.java.net/browse/JDK-8185886

Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
to improve listener removal speed.

This was due to the removal of listeners in TableView taking up to 50% of CPU 
time on the JavaFX Application thread when scrolling/adding rows to TableViews.

This may alleviate some of the issues seen here:

TableView has a horrific performance with many columns #409
https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033

JDK-8088394 : Huge memory consumption in TableView with too many columns
JDK-8166956: JavaFX TreeTableView slow scroll performance
JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in horizontal 
direction

OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ columns
https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

-

Commit messages:
 - JDK-8185886

Changes: https://git.openjdk.java.net/jfx/pull/108/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jfx=108=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252936
  Stats: 240 lines in 4 files changed: 105 ins; 76 del; 59 mod
  Patch: https://git.openjdk.java.net/jfx/pull/108.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/108/head:pull/108

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[dannygonzalez]

On Tue, 8 Sep 2020 20:14:48 GMT, Kevin Rushforth  wrote:

>>> So I'd like to propose this third alternative, which would affect only 
>>> VirtualFlow and the controls that use it, but
>>> wouldn't have any impact in the rest of controls as the other two options 
>>> (as ExpressionHelper or Node listeners
>>> wouldn't be modified).
>> 
>> Given PR #185, which was mentioned above, (it isn't out for review yet, but 
>> I want to evaluate it), this would be a 4th
>> approach.
>> As long as this really doesn't introduce a leak, it seems promising.
>> 
>> I note that these are not mutually exclusive.
>> 
>> We should discuss this on the list and not just in one or more of of the 3 
>> (soon to be 4) open pull requests.
>
> @dannygonzalez Per [this 
> message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-September/027534.html)
>  on
> the openjfx-dev mailing list, I have filed a new JBS issue for this PR to 
> use. Please change the title to:
> 8252936: Optimize removal of listeners from ExpressionHelper.Generic

Thanks @kevinrushforth, I've changed the title.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 10:46:51 GMT, dannygonzalez 
 wrote:

>> The problem is that there are usually many nodes, but only very few scenes 
>> and windows, so registering a listener for
>> each node on a scene or window is pretty bad design (also consider the 
>> amount of notifications that a scene change
>> would trigger in such scenarios).  As far as I can see, this is the only 
>> such listener and only needed for two very
>> limited cases, and its addition in the past may have slipped through the 
>> cracks.  Adding a performance unit test that
>> specifically checks add/remove performance of nodes may prevent such future 
>> regressions.
>
> @hjohn, agreed regards the issues of adding a listener to each node.
> 
> Would it be worth doing the additional work of updating PopupWindow and 
> ProgressIndicatorSkin to add their own
> listeners to make this a pull request that can be reviewed officially?
> I await any further comments from @kevinrushforth et al.

I have attached a code sample. If you use OpenJFX 16-ea+1 and run visual VM and 
look at the hotspots in the JavaFX
thread, you can see that about 45% of the time in the JavaFX thread is spent in 
removeListener calls.

Note: In CPU settings of VisualVM, I removed all packages from the "Do not 
profile packages section".

[JavaFXSluggish.java.zip](https://github.com/openjdk/jfx/files/5130298/JavaFXSluggish.java.zip)

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 10:22:15 GMT, John Hendrikx  wrote:

>> @hjohn Thanks for looking into this. It looks like your changes do improve 
>> the issue with the JavaFX thread being
>> swamped with listener de-registrations. Looking at the JavaFX thread in 
>> VisualVM, the removeListener call has dropped
>> off the hotspots in the same way it did with my pull request.  I wasn't 
>> fully confident of making changes to the Node
>> hierarchy to remove listeners hence why I approached the issue from the 
>> other direction i.e. the obvious bottleneck
>> which was the listener de-registration slowness.  I do worry however that 
>> any changes down the road which add listeners
>> to the Node hierarchy again without fully understanding the implications 
>> would lead us to the same point we are now
>> where the slowness of listener de-registrations becomes an issue again. 
>> There are no tests that catch this scenario.  I
>> feel that ideally both solutions are needed but am happy to bow to the more 
>> experienced OpenJFX devs opinions here as I
>> know my changes may be more fundamental and hence risky.
>
> The problem is that there are usually many nodes, but only very few scenes 
> and windows, so registering a listener for
> each node on a scene or window is pretty bad design (also consider the amount 
> of notifications that a scene change
> would trigger in such scenarios).  As far as I can see, this is the only such 
> listener and only needed for two very
> limited cases, and its addition in the past may have slipped through the 
> cracks.  Adding a performance unit test that
> specifically checks add/remove performance of nodes may prevent such future 
> regressions.

@hjohn, agreed regards the issues of adding a listener to each node.

Would it be worth doing the additional work of updating PopupWindow and 
ProgressIndicatorSkin to add their own
listeners to make this a pull request that can be reviewed officially?

I await any further comments from @kevinrushforth et al.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 08:07:08 GMT, John Hendrikx  wrote:

>> @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
>
> @dannygonzalez I added a proof of concept here if you want to play with it: 
> https://github.com/openjdk/jfx/pull/185

@hjohn Thanks for looking into this. It looks like your changes do improve the 
issue with the JavaFX thread being
swamped with listener de-registrations. Looking at the JavaFX thread in 
VisualVM, the removeListener call has dropped
off the hotspots in the same way it did with my pull request.

I wasn't fully confident of making changes to the Node hierarchy to remove 
listeners hence why I approached the issue
from the other direction i.e. the obvious bottleneck which was the listener 
de-registration slowness.

I do worry however that any changes down the road which add listeners to the 
Node hierarchy again without fully
understanding the implications would lead us to the same point we are now where 
the slowness of listener
de-registrations becomes an issue again. There are no tests that catch this 
scenario.  I feel that ideally both
solutions are needed but am happy to bow to the more experienced OpenJFX devs 
opinions here as I know my changes may be
more fundamental and hence risky.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Fri, 17 Apr 2020 07:08:01 GMT, dannygonzalez 
 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.
>
> @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

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 16:15:20 GMT, John Hendrikx  wrote:

>> @hjohn I have 12136 change listeners when debugging our application as you 
>> suggested.
>> 
>> Please note that I see the issue when the TableView is having items added to 
>> it. If you just have a static TableView I
>> do not see the issue.
>> It is only when you add items to the TableView which causes a myriad of 
>> listeners to be deregistered and registered.
>> The Visual VM snapshot I attached above was taken as our application was 
>> adding items to the TableView.
>
> 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.

@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

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 08:46:26 GMT, John Hendrikx  wrote:

>> If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
>> ExpressionHelper.removeListener is using
>> 61% of the JavaFX thread whilst running our application.
>> [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)
>> 
>> If you show only the JavaFX Application thread, press the "HotSpot" and 
>> "Reverse Calls" button you can take a look to
>> see which classes are calling the removeListener function.
>> ![Screenshot 2020-04-16 at 09 16
>> 11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)
>
> @dannygonzalez  Could you perhaps debug your application and take a look at 
> how large the following array is: a random
> node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> 
> `changeListeners`.  I just tested this with
> a custom control displaying 200 cells on screen at once (each cell consisting 
> of about 30 nodes itself), and I saw
> about 2 change listeners registered on this single Scene Window property. 
>  However, this custom control is not
> creating/destroying cells beyond the initial allocation, so there wouldn't be 
> any registering and unregistering going
> on, scrolling was still smooth >30 fps.

@hjohn I have 12136 change listeners when debugging our application as you 
suggested.

Please note that I see the issue when the TableView is having items added to 
it. If you just have a static TableView I
do not see the issue.

It is only when you add items to the TableView which causes a myriad of 
listeners to be deregistered and registered.
The Visual VM snapshot I attached above was taken as our application was adding 
items to the TableView.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 16 Apr 2020 07:34:40 GMT, John Hendrikx  wrote:

>> Looking at the commit  
>> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd
>>  it seems that the long listener lists are actually part of the `Scene`'s 
>> `Window` property and the `Window`'s `Showing`
>>  property.  Each `Node` registers itself on those and so the listener lists 
>> for those properties would scale with the
>>  number of nodes.
>> 
>> A test case showing this problem would really be great as then the patch can 
>> also be verified to solve the problem, but
>> I suppose it could be reproduced simply by having a large number of Nodes in 
>> a scene.  @dannygonzalez could you give us
>> an idea how many Nodes we're talking about?  1000? 10.000?  It also means 
>> there might be other options, do Nodes really
>> need to add these listeners and for which functionality and are there 
>> alternatives?  It would also be possible to
>> target only these specific properties with an optimized listener list to 
>> reduce the impact of this change.
>
> The listeners added by `Node` are apparently internally required for internal 
> properties `TreeShowing` and
> `TreeVisible`, and are used to take various decisions like whether to 
> play/pause animations.  There is also a couple of
> listeners registering on these properties in turn (in `PopupWindow`, 
> `SwingNode`, `WebView` and `MediaView`).  A lot of
> the checks for visibility / showing could easily be done by using the `Scene` 
> property and checking visibility /
> showing status from there.  No need for so many listeners.  The other classes 
> mentioned might register their own
> listener, instead of having `Node` do it for them (and thus impacting *every* 
> node).  Alternatively, `Node` may lazily
> register the listeners for Scene.Window and Window.Showing only when needed 
> (which from what I can see is for pretty
> specific classes only, not classes that you'd see a lot in a TableView...)

If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the 
ExpressionHelper.removeListener is using
61% of the JavaFX thread whilst running our application.

[snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip)

If you show only the JavaFX Application thread, press the "HotSpot" and 
"Reverse Calls" button you can take a look to
see which classes are calling the removeListener function.

![Screenshot 2020-04-16 at 09 16
11](https://user-images.githubusercontent.com/6702882/79432046-2f788800-7fc3-11ea-930a-98fed0190628.png)

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 14 Apr 2020 12:20:10 GMT, John Hendrikx  wrote:

>> In a minimal test I wrote (not a microbenchmark that removes listeners), I 
>> tried this PR code, but did not reproduce
>> the performance improvement. I have attached a test program in my PR(#125)
>
> @dannygonzalez You mentioned "There are multiple change listeners on a Node 
> for example, so you can imagine with the
> number of nodes in a TableView this is going to be a problem if the 
> unregistering is slow.".
> Your changes in this PR seem to be focused on improving performance of 
> unregistering listeners when there are thousands
> of listeners listening on changes or invalidations of the **same** property.  
> Is this actually the case?  Is there a
> single property in TableView or TreeTableView where such a high amount of 
> listeners are present?  If so, I think the
> problem should be solved there.  As it is, I donot think this PR will help 
> unregistration performance of listeners when
> the listeners are  spread out over dozens of properties, and although high in 
> total number, the total listeners per
> property would be low and their listener lists would be short.  Performance 
> of short lists (<50 entries) is almost
> unbeatable, so it is relevant for this PR to know if there are any properties 
> with long listener lists.

@hjohn
I haven't quantified exactly where all the listeners are coming from but the 
Node class for example has various
listeners on it.

The following changeset  (which added an extra listener on the Node class) 
impacted TableView performance significantly
(although it was still bad before this change in my previous tests):

> commit e21606d3a1b73cd4b44383babc750a4b4721edfd
> Author: Florian Kirmaier mailto:fk at sandec.de>>
> Date:   Tue Jan 22 09:08:36 2019 -0800
> 
> 8216377: Fix memoryleak for initial nodes of Window
> 8207837: Indeterminate ProgressBar does not animate if content is added 
> after scene is set on window
> 
> Add or remove the windowShowingChangedListener when the scene changes

As you can imagine, a TableView with many columns and rows can be made up of 
many Node classes. The impact of having
multiple listeners deregistering on the Node when new rows are being added to a 
TableView can be a significant
performance hit on the JavaFX thread.

I don't have the time or knowledge to investigate why these listeners are all 
needed especially around the Node class.
I went directly to the source of the problem which was the linear search to 
deregister each listener.

I have been running my proposed fix in our JavaFX application for the past few 
months and it has saved it from being
unusable due to the JavaFx thread being swamped.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 27 Feb 2020 03:17:14 GMT, yosbits 
 wrote:

>>> 
>>> 
>>> I think that a starting point would be to decide on a spec for the listener 
>>> notification order: is it specified by their registration order, or that is 
>>> it unspecified, in which case we can take advantage of this for better 
>>> performance. Leaving is unspecified and restricting ourselves to having it 
>>> ordered is losing on both sides.
>> 
>> technically true - but the implementation was linear with a fixed sequence 
>> since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans 
>> properties, even exposed as api to access the array of listeners). So 
>> technically, we could go the path of explicitely spec'ing that the order is 
>> unspecified. Pretty sure that doing so and implementing it will break tons 
>> of application code that's subtly relying on fifo notification (f.i. 
>> register before or after skin has its own is a simple wide-spread trick) .. 
>> which all did it wrong and might deserve it ;-) But then if even internal 
>> code does it ..
>
> In use cases where a large number of listeners are being discarded, it may be 
> better to first consider changing the design to receive event notifications 
> on nodes whose listener registrations are not frequently discarded.

> Hmm .. personally, I would consider changing such a basic (and probably 
> highly optimized) implementation used all over the framework is a very high 
> risk change that at the worst outcome would detoriate internal and external 
> code. So before going that lane, I would try - as you probably have, just me 
> not seeing it :) - to tackle the problem from the other end:
> 
> > I know that in our application, the **thousands of listeners** 
> > unregistering when using a TableView was stalling the JavaFX thread.
> 
> .. sounds like a lot. Any idea, where exactly they come into play?

I did start to look at why there were so many change listeners unregistering 
but felt that would take a deeper understanding of the architecture and design 
decisions of JavaFX scene graph and I didn't have that time to invest. 
I do know that there are thousands of them unregistering in a TableView and 
unregistering them is a bottleneck for the JavaFX thread.

There are multiple change listeners on a Node for example, so you can imagine 
with the number of nodes in a TableView this is going to be a problem if the 
unregistering is slow.

To get our application usable I profiled the code and saw this unregistering as 
a major bottleneck, hence why I looked at this more obvious solution.

I'm happy to look at the problem from the other angle and happy to listen to 
suggestions from people with more experience of the design and architecture but 
tackling the problem from the perspective of re-architecting the behaviour of 
listeners in the scene graph would be more work than I could feasibly take on.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 13:55:50 GMT, Nir Lisker  wrote:

>> Replying to @nlisker and @kevinrushforth general comments about the memory 
>> consumption of using a LinkedHashMap:
>> 
>> I agree that at the very least these should be lazy initialised when they 
>> are needed and that we should initially allocate a small capacity and load 
>> factor.
>> 
>> We could also have some sort of strategy where we could use arrays or lists 
>> if the number of listeners are less than some threshold (i.e. keeping the 
>> implementation with a similar overhead to the original) and then switch to 
>> the HashMap when it exceeds this threshold. This would make the code more 
>> complicated and I wonder if this is the worth the extra complexity.
>>  
>> I know that in our application, the thousands of listeners unregistering 
>> when using a TableView was stalling the JavaFX thread. 
>> 
>> I am happy to submit code that lazy initialises and minimises initial 
>> capacity and load factor as suggested or happy to take direction and 
>> suggestions from anyone with a deeper understanding of the code than myself.
> 
> I think that a starting point would be to decide on a spec for the listener 
> notification order: is it specified by their registration order, or that is 
> it unspecified, in which case we can take advantage of this for better 
> performance. Leaving is unspecified and restricting ourselves to having it 
> ordered is losing on both sides.

Even though the order of notifications is unspecified, the following tests fail 
if we use a HashMap vs LinkedHashMap i.e. the notifications are not in order of 
registration:

test.javafx.scene.control.TableViewTest > 
ensureSelectionIsCorrectWhenItemsChange FAILED
java.lang.AssertionError: expected:<0> but was:<-1>
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:470)
at org.junit.Assert.assertEquals(Assert.java:454)
at 
test.javafx.scene.control.TableViewTest.ensureSelectionIsCorrectWhenItemsChange(TableViewTest.java:333)

test.javafx.scene.control.TreeTableViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeTableViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeTableViewTest.java:2042)

test.javafx.scene.control.TreeViewTest > test_rt27185 FAILED
java.lang.AssertionError: expected: but 
was:
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:145)
at 
test.javafx.scene.control.TreeViewTest.test_rt27185(TreeViewTest.java:603)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_collapseBranch_laterSiblingAndChildrenSelected FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_collapseBranch_laterSiblingAndChildrenSelected(TreeViewTest.java:973)

test.javafx.scene.control.TreeViewTest > 
test_rt27180_expandBranch_laterSiblingSelected_singleSelection FAILED
java.lang.AssertionError: 
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.assertTrue(Assert.java:43)
at org.junit.Assert.assertTrue(Assert.java:54)
at 
test.javafx.scene.control.TreeViewTest.test_rt27180_expandBranch_laterSiblingSelected_singleSelection(TreeViewTest.java:992)


These would need to be investigated to see why they assume this order.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:43:34 GMT, Kevin Rushforth  wrote:

>> Sorry for the interruption, send a PR that corrects the same problem.
> 
> I haven't done a detailed review yet, but I worry about the memory 
> consumption and performance of using a Map for the simple cases where there 
> is only a single (or very small number) of listeners. These methods are used 
> in many more places than just the TableView / TreeTableView implementation.

Replying to @nlisker and @kevinrushforth general comments about the memory 
consumption of using a LinkedHashMap:

I agree that at the very least these should be lazy initialised when they are 
needed and that we should initially allocate a small capacity and load factor.

We could also have some sort of strategy where we could use arrays or lists if 
the number of listeners are less than some threshold (i.e. keeping the 
implementation with a similar overhead to the original) and then switch to the 
HashMap when it exceeds this threshold. This would make the code more 
complicated and I wonder if this is the worth the extra complexity.
 
I know that in our application, the thousands of listeners unregistering when 
using a TableView was stalling the JavaFX thread. 

I am happy to submit code that lazy initialises and minimises initial capacity 
and load factor as suggested or happy to take direction and suggestions from 
anyone with a deeper understanding of the code than myself.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Tue, 25 Feb 2020 00:45:06 GMT, Kevin Rushforth  wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 194:
>> 
>>> 193: private Map 
>>> invalidationListeners = new LinkedHashMap<>();
>>> 194: private Map, Integer> 
>>> changeListeners = new LinkedHashMap<>();
>>> 195: private T currentValue;
>> 
>> Two comments on this:
>> 
>> 1. The old implementation initialized these lazily, so we might want to 
>> preserve that behavior. I think it is reasonable since in most cases an 
>> observable won't have both types of listeners attached to it.
>> 2. Since we're dealing wither performance optimizations here, we should 
>> think about what the `initialCapcity` and `loadFactor` of these maps should 
>> be, as it can greatly increase performance when dealing with a large amount 
>> of entries.
> 
> Adding to this, we need to ensure that the simple case of a few listeners 
> doesn't suffer memory bloat. It may make sense to initially allocate a Map 
> with a small capacity and load factor, and then reallocate it if someone adds 
> more than a certain number of listeners.

Agree with the lazy initialisation and the initial setting of capacity and load 
factor to reduce memory footprint unless it's needed.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 11:32:25 GMT, Nir Lisker  wrote:

>> Nope, actually:
>> `listeners.keySet().removeIf(p::test) `
>> 
>> is not the same as:
>> `listeners.entrySet().removeIf(e -> p.test(e.getKey()));`
>> 
>> We need to test against the entrySet.key not the entrySet itself.
> 
>> We need to test against the entrySet.key not the entrySet itself.
> 
> I suggested to test against the elements in `keySet()`, which are the same as 
> the ones in `entrySet().getKey()`.

Gotcha, sorry I missed that.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 08:11:40 GMT, dannygonzalez 
 wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>>  line 64:
>> 
>>> 63: 
>>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>>> 65: }
>> 
>> This can be `listeners.keySet().removeIf(p::test);`.
> 
> Agreed, will change.

Nope, actually:
`listeners.keySet().removeIf(p::test) `

is not the same as:
`listeners.entrySet().removeIf(e -> p.test(e.getKey()));`

We need to test against the entrySet.key not the entrySet itself.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 10:16:55 GMT, dannygonzalez 
 wrote:

>>> So we need to keep the original behaviour from before where a count was 
>>> checked on every addListener call and the weak references were purged from 
>>> the array using the original calculation for this count.
>> 
>> The GC'd weak listeners do need to be purged, but why is the original 
>> behavior of the counters still applicable?
> 
> Just wanted to keep a similar behaviour as before using the same calculation 
> based originally on the size of the listeners array list and now based on the 
> size of the map. So in theory the weak listeners should be trimmed at the 
> same rate as before.
> Happy to hear alternatives.

Also bear in mind that the trimming of weak listeners is extremely slow as it 
has to iterate through each listener performing the weak listener test. We 
can't call this every time we add a listener as this would lock up the JavaFX 
thread completely (I tried it). 
I assume this is why the original calculation was used where it backs of the 
rate the weak listener trimming code was called as the array list grew.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 09:59:28 GMT, Nir Lisker  wrote:

>> As far as I know a LinkedHashMap doesn't automatically remove weak listener 
>> entries. The listener maps can be holding weak listeners as well as normal 
>> listeners.
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
>> Otherwise the map would never garbage collect these weak references.
>> 
>> The initial value of 2 for these counts was just the starting count although 
>> this is not necessarily strictly accurate. To be completely accurate then we 
>> would have to set the appropriate count in each constructor as follows:
>> 
>> i.e. in the Constructor with 2 InvalidationListeners:
>> weakChangeListenerGcCount = 0
>> weakInvalidationListenerGcCount = 2
>> 
>> in the Constructor with 2 ChangeListeners:
>> weakChangeListenerGcCount = 2
>> weakInvalidationListenerGcCount = 0
>> 
>> in the Constructor with 1 InvalidationListener and 1 ChangeListener:
>> weakChangeListenerGcCount = 1
>> weakInvalidationListenerGcCount = 1
>> 
>> Now, I could have used a WeakHashMap to store the listeners where it would 
>> automatically purge weak listeners but this doesn't maintain insertion 
>> order. Even though the specification doesn't mandate that listeners should 
>> be called back in the order they are registered, the unit tests failed when 
>> I didn't maintain order.
>> 
>> I am happy to remove this weak listener purge code (as it would make the 
>> code much simpler) but then we wouldn't automatically remove the weak 
>> listeners, but this may not be deemed a problem anyway?
> 
>> So we need to keep the original behaviour from before where a count was 
>> checked on every addListener call and the weak references were purged from 
>> the array using the original calculation for this count.
> 
> The GC'd weak listeners do need to be purged, but why is the original 
> behavior of the counters still applicable?

Just wanted to keep a similar behaviour as before using the same calculation 
based originally on the size of the listeners array list and now based on the 
size of the map. So in theory the weak listeners should be trimmed at the same 
rate as before.
Happy to hear alternatives.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Mon, 24 Feb 2020 08:14:02 GMT, dannygonzalez 
 wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>>  line 197:
>> 
>>> 196: private int weakChangeListenerGcCount = 2;
>>> 197: private int weakInvalidationListenerGcCount = 2;
>>> 198: 
>> 
>> Why are these set to 2 and why do you need them at all? The previous 
>> implementation needed to grow and shrink the array so it had to keep these, 
>> but `Map` takes care of this for you.
> 
> I agree, I kept these in as I wasn't sure if there was a need to manually 
> force the garbage collection of weak listeners at the same rate as the 
> original implementation.
> Removing this would make sense to me also.
> 
> Updated my thoughts on this, see my comments below.

As far as I know a LinkedHashMap doesn't automatically remove weak listener 
entries. The listener maps can be holding weak listeners as well as normal 
listeners.
So we need to keep the original behaviour from before where a count was checked 
on every addListener call and the weak references were purged from the array 
using the original calculation for this count.
Otherwise the map would never garbage collect these weak references.

The initial value of 2 for these counts was just the starting count although 
this is not necessarily strictly accurate. To be completely accurate then we 
would have to set the appropriate count in each constructor as follows:

i.e. in the Constructor with 2 InvalidationListeners:
weakChangeListenerGcCount = 0
weakInvalidationListenerGcCount = 2

in the Constructor with 2 ChangeListeners:
weakChangeListenerGcCount = 2
weakInvalidationListenerGcCount = 0

in the Constructor with 1 InvalidationListener and 1 ChangeListener:
weakChangeListenerGcCount = 1
weakInvalidationListenerGcCount = 1

Now, I could have used a WeakHashMap to store the listeners where it would 
automatically purge weak listeners but this doesn't maintain insertion order. 
Even though the specification doesn't mandate that listeners should be called 
back in the order they are registered, the unit tests failed when I didn't 
maintain order.

I am happy to remove this weak listener purge code (as it would make the code 
much simpler) but then we wouldn't automatically remove the weak listeners, but 
this may not be deemed a problem anyway?

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 01:05:25 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 283:
> 
>> 282: final Map 
>> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
>> 283: final Map, Integer> curChangeList 
>> = new LinkedHashMap<>(changeListeners);
>> 284: 
> 
> You only need the entry set, so you don't need to copy the map, just the set.

Thanks, yes the EntrySet would make more sense here. I'll fix that up.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 02:54:55 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 285:
> 
>> 284: 
>> 285: curInvalidationList.entrySet().forEach(entry -> 
>> fireInvalidationListeners(entry));
>> 286: 
> 
> The lambda can be converted to a method reference.

Agreed,  I'll fix.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 03:09:28 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelperBase.java
>  line 64:
> 
>> 63: 
>> 64: listeners.entrySet().removeIf(e -> p.test(e.getKey()));
>> 65: }
> 
> This can be `listeners.keySet().removeIf(p::test);`.

Agreed, will change.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Sun, 23 Feb 2020 04:20:55 GMT, Nir Lisker  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than 
>> Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of 
>> CPU time on the JavaFX Application thread when scrolling/adding rows to 
>> TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
>> horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
>> columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
> 
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ExpressionHelper.java
>  line 197:
> 
>> 196: private int weakChangeListenerGcCount = 2;
>> 197: private int weakInvalidationListenerGcCount = 2;
>> 198: 
> 
> Why are these set to 2 and why do you need them at all? The previous 
> implementation needed to grow and shrink the array so it had to keep these, 
> but `Map` takes care of this for you.

I agree, I kept these in as I wasn't sure if there was a need to manually force 
the garbage collection of weak listeners at the same rate as the original 
implementation.
Removing this would make sense to me also.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Wed, 12 Feb 2020 12:22:15 GMT, Jeanette Winzenburg  
wrote:

>>> hmm ... wouldn't the change violate spec of adding listeners:
>>> 
>>> > If the same listener is added more than once, then it will be notified 
>>> > more than once.
>> 
>> True, I hadn't read that spec in ObservableValueBase. 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
> 
>> 
>> Although that does seem odd behaviour to me. Obviously as the original 
>> implementation was using an array I can see how the implementation drove 
>> that specification.
>> 
> 
> whatever drove it (had been so since the beginning ot java desktop, at least 
> since the days of swing), there is no way to change it, is it?
> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests 
>> all passed. It would be nice if there was a specific test case for this 
>> behaviour.
>> 
> 
> yeah, the test coverage is ... not optimal :)
> 
>> I would need to store a registration count for each listener to satisfy this 
>> requirement.
> 
> a count plus some marker as to where it was added: 
> 
>addListener(firstL)
>addListener(secondL)
>addListener(firstL)
> 
> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
> which brings us back to .. an array?

The listeners are called back in the order they were registered in my 
implementation although I didn’t see this requirement in the spec unless I 
missed something.

The performance unregistering thousands of listeners by TableView from the 
array is killing the performance of our JavaFX application. It takes up to 60% 
of JavaFX Application thread CPU and there are various bug reports around this 
same TableView performance issue.
The set implementation has lowered this to below 1%.

This pull request may be too fundamental a change to go into mainline. We 
however have to build our local OpenJFX with this fix or our application is 
unusable.

I’m happy to receive any suggestions.

Danny


On 12 Feb 2020, at 12:22, Jeanette Winzenburg 
mailto:notificati...@github.com>> wrote:


Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

whatever drove it (had been so since the beginning ot java desktop, at least 
since the days of swing), there is no way to change it, is it?

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

yeah, the test coverage is ... not optimal :)

I would need to store a registration count for each listener to satisfy this 
requirement.

a count plus some marker as to where it was added:

addListener(firstL)
addListener(secondL)
addListener(firstL)

must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. 
which brings us back to .. an array?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on 
GitHub,
 or 
unsubscribe.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Wed, 12 Feb 2020 11:29:24 GMT, Jeanette Winzenburg  
wrote:

>> /signed
>> 
>> I have signed the Oracle Contributor Agreement today. Have not received back 
>> any confirmation yet though.
> 
> hmm ... wouldn't the change violate spec of adding listeners:
> 
>> If the same listener is added more than once, then it will be notified more 
>> than once.

> hmm ... wouldn't the change violate spec of adding listeners:
> 
> > If the same listener is added more than once, then it will be notified more 
> > than once.

True, I hadn't read that spec in ObservableValueBase. 
Although that does seem odd behaviour to me. Obviously as the original 
implementation was using an array I can see how the implementation drove that 
specification.

Non of the JavaFx unit tests test for that specific case as the unit tests all 
passed. It would be nice if there was a specific test case for this behaviour.

I would need to store a registration count for each listener to satisfy this 
requirement.

-

PR: https://git.openjdk.java.net/jfx/pull/108

Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Thu, 6 Feb 2020 16:13:33 GMT, dannygonzalez 
 wrote:

> https://bugs.openjdk.java.net/browse/JDK-8185886
> 
> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
> to improve listener removal speed.
> 
> This was due to the removal of listeners in TableView taking up to 50% of CPU 
> time on the JavaFX Application thread when scrolling/adding rows to 
> TableViews.
> 
> This may alleviate some of the issues seen here:
> 
> TableView has a horrific performance with many columns #409
> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
> 
> JDK-8088394 : Huge memory consumption in TableView with too many columns
> JDK-8166956: JavaFX TreeTableView slow scroll performance
> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in 
> horizontal direction
> 
> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ 
> columns
> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

/signed

I have signed the Oracle Contributor Agreement today. Have not received back 
any confirmation yet though.

-

PR: https://git.openjdk.java.net/jfx/pull/108

RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

https://bugs.openjdk.java.net/browse/JDK-8185886

Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays 
to improve listener removal speed.

This was due to the removal of listeners in TableView taking up to 50% of CPU 
time on the JavaFX Application thread when scrolling/adding rows to TableViews.

This may alleviate some of the issues seen here:

TableView has a horrific performance with many columns #409
https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033

JDK-8088394 : Huge memory consumption in TableView with too many columns
JDK-8166956: JavaFX TreeTableView slow scroll performance
JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in horizontal 
direction

OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ columns
https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html

-

Commits:
 - 05c37196: JDK-8185886

Changes: https://git.openjdk.java.net/jfx/pull/108/files
 Webrev: https://webrevs.openjdk.java.net/jfx/108/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8185886
  Stats: 240 lines in 4 files changed: 105 ins; 76 del; 59 mod
  Patch: https://git.openjdk.java.net/jfx/pull/108.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/108/head:pull/108

PR: https://git.openjdk.java.net/jfx/pull/108