Hi, I just wanted to clarify a few things about the fix. The test I showed hints at a potentially safer way to implement the fix for this problem. One would expect adding nodes 1 by 1 to the scene graph to be less efficient than adding them all at once, but it's far worse to add them all at once. The fix demonstrates that the test's performance problem comes from the reapplication of css, and is not necessarily the best fix for the problem, it's just the least impactful fix I could come up with, since the idea is that adding nodes one by one is safe already.
Reapplying CSS to a Node but not its children could cause a problem if > there are styles in the parent or the parent's parents that affect the > children. It seems like bypassing children in reapplyCSS is bound to > cause a regression. > CSS does get applied to the children. The problem was that each parent would call scenesChanged, which would then call scenesChanged on all the children recursively. As they're bubbling out they call reapplyCSS on themselves which calls reapplyCss on themselves and all their children. Jonathan's solution was to only allow the node that was added to the scene graph to reapplyCSS to its children, thus applying css in a top down way. I suspect with some modifications it could work. One thing it didn't do was call reapplyCSS (different from reapplyCss) on every node. It only called reapplyCss on every node, and there is some additional logic in reapplyCSS. (The commit which undoes this change is here: https://github.com/javafxports/openjdk-jfx/commit/cd14f72776281a2384c50a35e618f1fb258c9430#diff-4a65018af7ebb15827ff1c67a3c29e86 ). In mine, reapplyCSS happens first and doesn't propagate down to the children. Instead, the scenesChanged method will trigger invalidatedScenes on all the children, and they will reapplyCSS for themselves. Basically each node calls reapplyCSS for itself, then propagates. In this way css is applied top down in a way similar to adding the nodes 1 by 1. Also, only this one case where nodes are added all at once is affected. Kevin has also mentioned https://bugs.openjdk.java.net/browse/JDK-8173301 as a way to mitigate the problem. I think something like that would be fine for this particular css performance issue, but there are several other issues. It would be nice to have a safer way to work on these issues so that they could get tackled. I think there are several options here. I just wanted to share some insights, and perhaps contribute a test or two. Kevin mentioned "We will need a performance regression test to measure the gain." Are there any performance tests like this already I could look at as an example? Dean On Wed, Nov 7, 2018 at 3:37 PM David Grieve <david.gri...@oracle.com> wrote: > One of the dangers of mucking around with the CSS code is whether or not > the changes break things like popups, dialogs, menus. And whether or not > the change breaks inline styles versus attributes set in code, versus > stylesheets added to the scene/subscene/control, versus default > stylesheets. And whether or not the changes breaks skins for controls. > > Reapplying CSS to a Node but not its children could cause a problem if > there are styles in the parent or the parent's parents that affect the > children. It seems like bypassing children in reapplyCSS is bound to > cause a regression. > > Also, because a new scene root could affect styles, CSS is reapplied > after calling sceneChanged - your change puts it before, so I question > whether or not this will cause a regression. I think if you skip > reapplying CSS when a Node's scene has changed, then the children won't > get the correct styles, and this will affect layout. > > I haven't spent much time evaluating this change in detail, but I'm > doubtful that it won't cause regressions. > >
