Re: RFR: 8252935: Add treeShowing listener only when needed [v3]
On Mon, 1 Feb 2021 04:41:20 GMT, John Hendrikx wrote: >> The updated fix using `registerChangeListener` and the test look good. I >> left a few mostly-minor comments. > > I think this change is complete now, including an integration test and tests > on all new code. Please let me know if anything else needs to be done. This PR also seems to fix another issue [JDK-8259558](https://bugs.openjdk.java.net/browse/JDK-8259558). Test program is attached to the bug. Time readings, with this fix, time to remove 10 nodes : ~20 ms time to add those removed 10 nodes to a different parent: ~80 ms without this fix, time to remove 10 nodes : ~1720 ms time to add those removed 10 nodes to a different parent: ~100 ms - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed [v3]
On Thu, 28 Jan 2021 13:37:59 GMT, Kevin Rushforth wrote: >> John Hendrikx has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Add performance test >> - Use registerChangeListener in ProgressIndicatorSkin > > The updated fix using `registerChangeListener` and the test look good. I left > a few mostly-minor comments. I think this change is complete now, including an integration test and tests on all new code. Please let me know if anything else needs to be done. > modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights >> reserved. > > change 2020 to 2021 Solved, and also did this in all other files included in this change. - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed [v3]
On Sun, 24 Jan 2021 15:25:06 GMT, John Hendrikx wrote:
>> This is a PoC for performance testing.
>>
>> It contains commented out code in PopupWindow and ProgressIndicatorSkin and
>> two tests are failing because of that.
>>
>> This code avoids registering two listeners (on Scene and on Window) for each
>> and every Node to support the aforementioned classes. The complete change
>> should update these two classes to add their own listeners instead.
>
> John Hendrikx has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Add performance test
> - Use registerChangeListener in ProgressIndicatorSkin
The updated fix using `registerChangeListener` and the test look good. I left a
few mostly-minor comments.
tests/system/src/test/java/test/javafx/scene/NodeTreeShowingTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
2021
tests/system/src/test/java/test/javafx/scene/NodeTreeShowingTest.java line 125:
> 123: });
> 124:
> 125: for(int j = 0; j < 5; j++) {
MInor: add space after `for`
tests/system/src/test/java/test/javafx/scene/NodeTreeShowingTest.java line 49:
> 47: /**
> 48: * TODO fix
> 49: * This test is based on the test case reported in JDK-8209830
Comment block needs to be updated to describe this bug.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java
line 2:
> 1: /*
> 2: * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights
> reserved.
change 2020 to 2021
-
PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed [v3]
On Sun, 24 Jan 2021 09:02:17 GMT, John Hendrikx wrote: >> I left a few inline comments below. > > Sorry about the force push, merging over a year of changes from master did > not seem right to me. It was only for getting up to date, I will do the > other commits normally. > 1. Merge in the latest changes from the upstream master branch into your > local feature branch? Among other things, this will enable running tests via > GitHub Actions. I hadn't seen that in practice yet, it looks really nice. > 2. Add a test program under `tests/performance` that can be used to verify > the performance gain. Even better would be if you can create an automated > test under `tests/system`. I was thinking something like what we did for PR > #34 where there was a pathological performance bug fixed and the test checked > that the operation in question didn't take more than some small number of > seconds. That might be overkill for this fix, though. I put a test under `tests/system` using the example you gave. It creates a Scene with about 16k nodes, and then does 10.000 operations on them. This runs in around 2-2.5 seconds before the fix and in about 100-200 ms after. I put the cut-off point somewhere in the middle (800 ms). I am planning to address the rest of the comments somewhere in the coming week. - PR: https://git.openjdk.java.net/jfx/pull/185
Re: RFR: 8252935: Add treeShowing listener only when needed [v3]
> This is a PoC for performance testing. > > It contains commented out code in PopupWindow and ProgressIndicatorSkin and > two tests are failing because of that. > > This code avoids registering two listeners (on Scene and on Window) for each > and every Node to support the aforementioned classes. The complete change > should update these two classes to add their own listeners instead. John Hendrikx has updated the pull request incrementally with two additional commits since the last revision: - Add performance test - Use registerChangeListener in ProgressIndicatorSkin - Changes: - all: https://git.openjdk.java.net/jfx/pull/185/files - new: https://git.openjdk.java.net/jfx/pull/185/files/5ac5d9e4..e68a886d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=185=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=185=01-02 Stats: 170 lines in 2 files changed: 167 ins; 2 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/185.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/185/head:pull/185 PR: https://git.openjdk.java.net/jfx/pull/185
