th 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.
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 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
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 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 weakChangeListenerGcCou
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
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
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 notifica
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
d (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/open
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
he removeListener function.
>>
>> 
>
> @dannygonzalez Could you perhaps debug your application and take a look at
> how
lso 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
is 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
>&g
ed, 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=ABTEOIWBBLX3XA3JV23OP6LRCPSXRA5CNFSM4KQ7YBNKYY3PNVWWK
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 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 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 T
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
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
oduce 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.ope
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 no
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
>> 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
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
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
eListener function.
>> 
>
> @dannygonzalez Could you perhaps debug your application and take a look at
> how large the following arr
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
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)
>
> @dan
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
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 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>
>>>
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
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()));
>>>
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
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 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 weakInva
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 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
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
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
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 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
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 T
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
48 matches
Mail list logo
