RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Jeanette Winzenburg]

On Wed, 26 Feb 2020 09:49:04 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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:<TreeItem [ value: Mike Graham ]> but was:<null>
>>         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.
> 
> 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 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 ..

-------------

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