On Sat, 2 Oct 2021 13:35:51 GMT, Kevin Rushforth wrote:
>> The behavior is the same between openjdk 16.0.2 and 17.0.0.1. The test to
>> recreate the problem adds many nodes (Rectangle) to a parent Group. The
>> process causes each node to add two invalidation listeners (disabledProperty
>>
On Fri, 1 Oct 2021 21:11:01 GMT, John Nader
wrote:
> Here is the minimal application I am using to recreate the issue.
I'll attach the test case you provided to the bug report.
> The question now is should the work done on this PR be abandoned. It
> addresses a current performance limitation
On Wed, 14 Apr 2021 12:33:23 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
On Wed, 14 Apr 2021 12:33:23 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
On Wed, 14 Apr 2021 12:33:23 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
On Fri, 2 Apr 2021 10:45:47 GMT, yosbits
wrote:
>> dannygonzalez has updated the pull request incrementally.
>
> It seems that performance improvement PR often needs to make up for the lack
> of existing unit tests. However, this addition of unit tests should not be
> included in the same PR
On Wed, 14 Apr 2021 12:33:23 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
> 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
On Thu, 1 Apr 2021 23:50:12 GMT, Marius Hanl
wrote:
>> 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
On Thu, 10 Sep 2020 15:49:59 GMT, yosbits
wrote:
>> @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
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
>
On Mon, 24 Feb 2020 11:04:26 GMT, dannygonzalez
wrote:
>> 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
>>
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>
>>>
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
On Mon, 24 Feb 2020 23:45:03 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
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
On Mon, 24 Feb 2020 09:14:51 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
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
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
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
On Thu, 10 Sep 2020 09:48:07 GMT, yosbits
wrote:
>> Thanks @kevinrushforth, I've changed the title.
>
> 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
On Wed, 26 Aug 2020 15:40:57 GMT, Jose Pereda 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
On Thu, 10 Sep 2020 13:41:00 GMT, Kevin Rushforth wrote:
>> 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.**
>>
On Wed, 9 Sep 2020 06:53:11 GMT, dannygonzalez
wrote:
>> @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:
>>
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:
On Wed, 26 Aug 2020 14:08:37 GMT, dannygonzalez
wrote:
>> @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
On Wed, 26 Aug 2020 17:44:20 GMT, yosbits
wrote:
>> 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
On Wed, 26 Aug 2020 15:40:57 GMT, Jose Pereda 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
On Wed, 26 Aug 2020 14:08:37 GMT, dannygonzalez
wrote:
>> @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
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
>>
On Fri, 17 Apr 2020 08:48:35 GMT, dannygonzalez
wrote:
>> @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
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
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
On Fri, 17 Apr 2020 07:22:20 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
>>
>>
On Thu, 16 Apr 2020 09:45:20 GMT, dannygonzalez
wrote:
>> @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
>>
On Thu, 16 Apr 2020 08:24:16 GMT, dannygonzalez
wrote:
>> 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
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
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.
>>
>>
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`
On Thu, 16 Apr 2020 07:11:58 GMT, John Hendrikx wrote:
>> 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
On Thu, 16 Apr 2020 06:34:39 GMT, yosbits
wrote:
>> @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
On Wed, 15 Apr 2020 07:11:27 GMT, dannygonzalez
wrote:
>> @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
On Wed, 26 Feb 2020 10:07:02 GMT, Jeanette Winzenburg
wrote:
> 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
>
On Mon, 2 Mar 2020 11:47:03 GMT, Nir Lisker 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
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
On Mon, 2 Mar 2020 08:31:47 GMT, dannygonzalez
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
>>
On Wed, 26 Feb 2020 10:07:02 GMT, Jeanette Winzenburg
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
On Tue, 25 Feb 2020 13:55:50 GMT, Nir Lisker 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
>
On Tue, 25 Feb 2020 14:06:23 GMT, dannygonzalez
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
>>
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
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
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
On Tue, 25 Feb 2020 12:18:34 GMT, dannygonzalez
wrote:
>> 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
On Sat, 22 Feb 2020 08:42:52 GMT, yosbits
wrote:
>> @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
>>
On Tue, 18 Feb 2020 09:02:36 GMT, dannygonzalez
wrote:
>> @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
>>
On Wed, 12 Feb 2020 12:43:58 GMT, dannygonzalez
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
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.
On Wed, 12 Feb 2020 12:43:58 GMT, dannygonzalez
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.
yeah, you are right can't find that spec on sequence right now -
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
On Wed, 12 Feb 2020 12:01:03 GMT, dannygonzalez
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
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
On Thu, 6 Feb 2020 16:22:28 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
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
On Thu, 10 Sep 2020 13:41:00 GMT, Kevin Rushforth wrote:
>> 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.**
>>
On Thu, 10 Sep 2020 09:48:07 GMT, yosbits
wrote:
>> Thanks @kevinrushforth, I've changed the title.
>
> 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
On Wed, 9 Sep 2020 06:53:11 GMT, dannygonzalez
wrote:
>> @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:
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
67 matches
Mail list logo
