Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8252936: Optimize removal of listeners from ExpressionHelper.Generic #108

Open
wants to merge 1 commit into
base: master
from

Conversation

@dannygonzalez
Copy link

dannygonzalez commented Feb 6, 2020

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
javafxports/openjdk-jfx#409 (comment)

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8252936: Optimize removal of listeners from ExpressionHelper.Generic

Download

$ git fetch https://git.openjdk.java.net/jfx pull/108/head:pull/108
$ git checkout pull/108

@bridgekeeper bridgekeeper bot added the oca label Feb 6, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 6, 2020

Hi dannygonzalez, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user dannygonzalez" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 6, 2020

/signed

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

@bridgekeeper bridgekeeper bot added the oca-verify label Feb 6, 2020
@bridgekeeper
Copy link

bridgekeeper bot commented Feb 6, 2020

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@kleopatra
Copy link
Collaborator

kleopatra commented Feb 12, 2020

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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 12, 2020

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.

@kleopatra
Copy link
Collaborator

kleopatra commented Feb 12, 2020

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?

@dannygonzalez dannygonzalez changed the title 8185886: Improve scrolling performance of TableView and TreeTableView WIP: Improve scrolling performance of TableView and TreeTableView Feb 12, 2020
@dannygonzalez dannygonzalez changed the title WIP: Improve scrolling performance of TableView and TreeTableView 8185886: Improve scrolling performance of TableView and TreeTableView Feb 12, 2020
@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 12, 2020

@kleopatra
Copy link
Collaborator

kleopatra commented Feb 12, 2020

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.

yeah, you are right can't find that spec on sequence right now - maybe I dreamed it :)

@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 12, 2020

/covered

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 12, 2020

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 17, 2020

/covered

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 17, 2020

You are already a known contributor!

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 17, 2020

@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.

ExpressionHelper.Generic now uses a Map for speed rather than linear
searching through an array.

We use a LinkedHashMap<Listener, Integer> in ExpressionHelper.Generic.
This ensures we can iterate through the map in insertion order to call back
listeners in the order they were registered (although this isn't a requirement
according to the spec, I got unit test failures when I used a HashMap instead).

It also allow us to keep track if the same listener has been registerd more than
once and hence honour the addListener and removeListener requirements.
Specifically:

addListener:
If the same listener is added more than once, then it will be notified more than once. That is, no check is made to ensure uniqueness.

removeLister:
If it had been added more than once, then only the first occurrence will be removed.

Check in unit tests to test the case where the same listener is registered/removed multiple times
@dannygonzalez dannygonzalez force-pushed the screamingfrog:listeners_optimisation branch from 249ab01 to 05c3719 Feb 18, 2020
@openjdk openjdk bot added the rfr label Feb 18, 2020
@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 18, 2020

@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

@mlbridge
Copy link

mlbridge bot commented Feb 18, 2020

Webrevs

@yososs
Copy link

yososs commented Feb 22, 2020

Sorry for the interruption, send a PR that corrects the same problem.

final Map<InvalidationListener, Integer> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
final Map<ChangeListener<? super T>, Integer> curChangeList = new LinkedHashMap<>(changeListeners);
Comment on lines +282 to +283

This comment has been minimized.

@nlisker

nlisker Feb 23, 2020 Collaborator

You only need the entry set, so you don't need to copy the map, just the set.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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

final Map<InvalidationListener, Integer> curInvalidationList = new LinkedHashMap<>(invalidationListeners);
final Map<ChangeListener<? super T>, Integer> curChangeList = new LinkedHashMap<>(changeListeners);

curInvalidationList.entrySet().forEach(entry -> fireInvalidationListeners(entry));

This comment has been minimized.

@nlisker

nlisker Feb 23, 2020 Collaborator

The lambda can be converted to a method reference.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

Agreed, I'll fix.

Predicate<Object> p = t -> t instanceof WeakListener &&
((WeakListener)t).wasGarbageCollected();

listeners.entrySet().removeIf(e -> p.test(e.getKey()));

This comment has been minimized.

@nlisker

nlisker Feb 23, 2020 Collaborator

This can be listeners.keySet().removeIf(p::test);.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

Agreed, will change.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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.

This comment has been minimized.

@nlisker

nlisker Feb 24, 2020 Collaborator

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().

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

Gotcha, sorry I missed that.

private int weakChangeListenerGcCount = 2;
private int weakInvalidationListenerGcCount = 2;
Comment on lines +196 to +197

This comment has been minimized.

@nlisker

nlisker Feb 23, 2020 Collaborator

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.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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?

This comment has been minimized.

@nlisker

nlisker Feb 24, 2020 Collaborator

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?

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 24, 2020 Author

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.

This comment has been minimized.

@nlisker

nlisker Feb 24, 2020 Collaborator

I honestly don't quite understand the old cleanup behavior of (oldCapacity * 3)/2 + 1. Why is it grown by x1.5? In your tests, can you try to change the cleanup threshold to higher and lower values and see what differences you get?

At the very least, the initial values of the counters should be set according to the specific constructor used.

private Map<InvalidationListener, Integer> invalidationListeners = new LinkedHashMap<>();
private Map<ChangeListener<? super T>, Integer> changeListeners = new LinkedHashMap<>();
Comment on lines +193 to +194

This comment has been minimized.

@nlisker

nlisker Feb 24, 2020 Collaborator

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.

This comment has been minimized.

@kevinrushforth

kevinrushforth Feb 25, 2020 Member

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.

This comment has been minimized.

@dannygonzalez

dannygonzalez Feb 25, 2020 Author

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

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 25, 2020

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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Feb 25, 2020

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.

@nlisker
Copy link
Collaborator

nlisker commented Feb 25, 2020

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.

@openjdk
Copy link

openjdk bot commented Apr 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@hjohn
Copy link
Collaborator

hjohn commented Apr 14, 2020

@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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 15, 2020

@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 e21606d
Author: Florian Kirmaier <fk at sandec.de<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.

@yososs
Copy link

yososs commented Apr 16, 2020

The patch proposed here does not share the case where the listener deletion performance becomes a bottleneck.

I think that it is necessary to reproduce it by testing first, but

If you just focus on improving listener removal performance,

If the lifespan of a large number of registered listeners is biased,
It seems like the next simple change can improve delete performance without changing the data structure.

  • Change the search from the front of the list to the search from the back.

This will reduce the number of long-life listeners matching.

@hjohn
Copy link
Collaborator

hjohn commented Apr 16, 2020

Looking at the commit e21606d
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.

@hjohn
Copy link
Collaborator

hjohn commented Apr 16, 2020

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...)

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 16, 2020

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

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

@hjohn
Copy link
Collaborator

hjohn commented Apr 16, 2020

@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 20000 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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 16, 2020

@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.

@hjohn
Copy link
Collaborator

hjohn commented Apr 16, 2020

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 e21606d 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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 17, 2020

@hjon

  1. 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: #108 (comment)

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 17, 2020

@hjohn

  1. 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: #108 (comment)

@hjohn
Copy link
Collaborator

hjohn commented Apr 17, 2020

I've implemented an alternative solution: Removing the listeners on Window.showingProperty and Scene.windowProperty completely. They are in fact only used in two places: PopupWindow in order to remove itself if the Window is no longer showing, and ProgressIndicatorSkin. These two can be easily replaced with their own listeners for these properties instead of burdening all nodes with these listeners only to support these two classes.

I left the isTreeShowing method in, and implemented it simply as isTreeVisible() && isWindowShowing() as that's really the only difference between "visible" and "showing" apparently.

Here is the test result with 20.000 nested StackPanes with only this change in:

  • Add all 45 ms
  • Remove all 25 ms

I think this might be a good solution as it completely avoids these listeners.

@hjohn
Copy link
Collaborator

hjohn commented Apr 17, 2020

@dannygonzalez I added a proof of concept here if you want to play with it: #185

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 17, 2020

@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.

@hjohn
Copy link
Collaborator

hjohn commented Apr 17, 2020

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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Apr 17, 2020

@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.

@dannygonzalez
Copy link
Author

dannygonzalez commented Aug 26, 2020

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

@jperedadnr
Copy link
Collaborator

jperedadnr commented Aug 26, 2020

So far, there are two alternative fixes for the bad performance issue while scrolling TableView/TreeTableViews:

  • this one #108, that tries to improve the performance of excessive number of removeListener calls
  • the WIP #185 that avoids registering two listeners (on Scene and on Window) for each and every Node.

For the case presented, where new items are constantly added to the TableView, the trace of calls that reach com.sun.javafx.binding.ExpressionHelper.removeListener() is something like this:

TraceOpenJFX16ea1

As can be seen, all those calls are triggered by the change of the number of cells in VirtualFlow::setCellCount.

Whenever the cell count changes there is this call:

sheetChildren.clear();

This happens every time the number of items in the TableView changes, as the VirtualFlow keeps track of the number of virtual cells (cellCount is the total number of items) while the number of actual cells or number of visible nodes used is given by sheetChildren.size().

This means that all the actual cells (nodes) that are used by the VirtualFlow are disposed and recreated all over again every time the number of items changes, triggering all those calls to unregister those nodes from the scene that ultimately lead to remove the listeners with ExpressionHelper.removeListener.

However, this seems unnecessary, as the number of actual cells/nodes doesn't change that much, and causes this very bad performance.

On a quick test over the sample posted, just removing that line gives a noticeable improvement in performance..

There is a concern though due to the comment:

// Fix for RT-13965: Without this line of code, the number of items in
// the sheet would constantly grow, leaking memory for the life of the
// application. This was especially apparent when the total number of
// cells changes - regardless of whether it became bigger or smaller.
sheetChildren.clear();

There are some methods in VirtualFlow that already manage the lifecycle of this list of nodes (clear, remove cells, add cells, ...), so I don't think that is the case anymore for that comment: the number of items in the sheet doesn't constantly grow and there is no memory leak.

Running the attached sample for a long time, and profiling with VisualVM, shows improved performance (considerable drop in CPU usage), and no issues regarding memory usage.

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).

Thoughts and feedback are welcome.

@yososs
Copy link

yososs commented Aug 26, 2020

I confirmed the sample code (JavaFX Sluggish),
This is not scroll performance
It seems to reproduce the additional performance issue.
Therefore, it is not considered appropriate as a fix for JDK-8185886.
I know you are reproducing another performance issue, but
I'm proposing to fix scrolling performance issues in #125.

@kevinrushforth kevinrushforth self-requested a review Aug 26, 2020
@hjohn
Copy link
Collaborator

hjohn commented Aug 26, 2020

The #185 is a full fix, not a WIP. It avoids registering the listeners on Scene and Window and moves the only uses of those listeners to their respective controls, PopupWindow and ProgressIndicatorSkin (the property involved is internal, so there is no risk of affecting 3rd parties).

Please have a closer look.

I updated the title to make it more clear it is no longer a WIP, unless someone has review comments.

@kevinrushforth
Copy link
Member

kevinrushforth commented Aug 27, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 8, 2020

@dannygonzalez Per this message 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
@dannygonzalez dannygonzalez changed the title 8185886: Improve scrolling performance of TableView and TreeTableView 8252936: Optimize removal of listeners from ExpressionHelper.Generic Sep 9, 2020
@dannygonzalez
Copy link
Author

dannygonzalez commented Sep 9, 2020

Thanks @kevinrushforth, I've changed the title.

@yososs
Copy link

yososs commented Sep 10, 2020

I have found that fixing this rudimentary problematic code alleviates your problem.

This fix will reduce CPU usage by about 1/3 without your changes.
This fix improves performance in many widespread use cases.

However, I'm wondering how to report the problem. Should it be handled in this issue? Should I deal with a new issue for a rudimentary issue?

@kevinrushforth What should i do?

@Override
public boolean removeAll(Collection<?> c) {
beginChange();
BitSet bs = new BitSet(c.size());
for (int i = 0; i < size(); ++i) {
if (c.contains(get(i))) {
bs.set(i);
}
}
if (!bs.isEmpty()) {
int cur = size();
while ((cur = bs.previousSetBit(cur - 1)) >= 0) {
remove(cur);
}
}
endChange();
return !bs.isEmpty();
}
@Override
public boolean retainAll(Collection<?> c) {
beginChange();
BitSet bs = new BitSet(c.size());
for (int i = 0; i < size(); ++i) {
if (!c.contains(get(i))) {
bs.set(i);
}
}
if (!bs.isEmpty()) {
int cur = size();
while ((cur = bs.previousSetBit(cur - 1)) >= 0) {
remove(cur);
}
}
endChange();
return !bs.isEmpty();
}

Rewritten so that BitSet is not used.

    @Override
    public boolean removeAll(Collection<?> c) {
    	if(this.isEmpty() || c.isEmpty()){
    		return false;
    	}
        beginChange();
        boolean removed = false;
        for (int i = size()-1; i>=0; i--) {
            if (c.contains(get(i))) {
                remove(i);
                removed = true;
            }
        }
        endChange();
        return removed;
    }

    @Override
    public boolean retainAll(Collection<?> c) {
    	if(this.isEmpty() || c.isEmpty()){
    		return false;
    	}
        beginChange();
        boolean retained = false;
        for (int i = size()-1; i>=0; i--) {
            if (!c.contains(get(i))) {
                remove(i);
                retained = true;
            }
        }
        endChange();
        return retained;
    }
@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 10, 2020

@yososs Please file a new JBS issue for this. You will need to prove that your proposed change is functionally equivalent (or that any perceived changes are incidental and still conform to the spec). You should also think about whether your proposed change needs additional tests.

@yososs
Copy link

yososs commented Sep 10, 2020

Because it is such a small correction
Problem from me I feel that it is not easy to register, but I will try to register.

It has passed two existing tests for compatibility:

  • gradle base:test
  • gradle controls:test

I have just reported it as an enhancement proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.