RFR: 8345188: Support tree-structural pseudo-classes [v6]
John Hendrikx
jhendrikx at openjdk.org
Wed Jan 8 22:11:41 UTC 2025
On Wed, 8 Jan 2025 17:39:10 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 479:
>>
>>> 477: n.pseudoClassStateChanged(NTH_EVEN_CHILD_PSEUDO_CLASS, i % 2 != 0);
>>> 478: n.pseudoClassStateChanged(NTH_ODD_CHILD_PSEUDO_CLASS, i % 2 == 0);
>>> 479: }
>>
>> With this new code, Nodes with many children being manipulated may incur some significant new costs now that these pseudo classes need to be changed for potentially many/all children. Where before adding a new first child mostly had a cost in shifting the child array with a copy, now we also potentially have two listener call backs per child (and I'm unsure, but these may even trigger some CSS recalculations or CSS cache invalidations). Nodes with many children may suffer from this, and mostly needlessly as these pseudo classes will see very limited use.
>>
>> I think there's some potential for performance regression here that is perhaps sufficient to warrant the exclusion of this feature or making it optional.
>
> I think this concern is out of proportion. Generally, nodes will not have such a huge number of children that simply flipping a few pseudo-classes will be prohibitively expensive. CSS recalculation will, in most cases, only happen once per pulse anyway.
>
> I don't know why we would be optimizing for pathological applications that register two pseudo-class listeners for each of their tens of thousands of children in a container, _and_ do heavy work in those listeners.
Perhaps it is not as dire, but it may be worth a short check how this works with say 1000 nodes with and without this new code (no need to register listeners, I agree that that is a different problem, I was more worried about how CSS handles this many changes). Perhaps also a case where there is also some style change going on linked to the odd or even pseudo class (although if that is much slower, then I guess that's what the user wanted...)
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907919550
More information about the openjfx-dev
mailing list