performance impact of JDK-8360940 in case of a wrong but common pattern

[John Hendrikx]

Johan, I had a closer look.

TLDR: 

- FX currently almost always does two layout passes, with the 2nd pass
not changing anything, due to a flaw in Node's relayout trigger logic
- I think it may be possible to fix this, and as a side effect
also fixes your infinite layout loop scenario 

Longer version:

Although I think the original fix is correct (as it fixes broken state
tracking), I do see that the fix may cause existing flaws, that were
swept under the rug before, to become more problematic.

I think the fundamental reason for the relayout trigger code in Node on
LayoutX/Y changes is to catch cases when these are modified OUTSIDE the
scope of its direct Parent's layoutChildren call. In fact, it seemingly
does its best to detect when a change occurs as part of a Parent's
layoutChildren call as seen in this code and the clarifying comment:

// Propagate layout if this change isn't triggered by its parent

if(p != null&& !p.isCurrentLayoutChild(Node.this)) {

However, this check utterly fails in almost all situations, for the
simple reason that the child node which has its layout X/Y changed is
NOT considered the current layout child by its parent.  None of the
containers will update the current layout child before doing each
layoutX/Y change (as that would be very cumbersome...)  However, we're
not actually interested in that.  All we want to know is: 

     *** Is this layout X/Y change performed while the direct
Parent's layoutChildren() call is higher up in the call stack? ***

There are two ways to answer that question:

- One can ask the *grandparent* if the child's parent is the current
layout child -> the answer would be yes, BUT it only means its
layout() is running, and often also its layoutChildren() (so this isn't
perfect)
- Another option is to ask the direct parent if it is currently
performing layout -> the answer would be yes, and it normally means its
layoutChildren is running, but the flag isn't cleared immediately after
the call completes, so this is also a bit flawed; excerpt from
Parent::layout():

performingLayout= true;

layoutChildren(); ... (more code that triggers layouts for the rest of
the sub tree) ...

performingLayout= false;

The flag however is incredibly close to what we need, and an extra
tighter flag that is reset immediately after `layoutChildren` completes
(instead of after the whole subtree completes layout) would be exactly
what we need.

So now if Node wants to know if a layout X/Y is changed because the
parent's layout children triggered it, then all it needs to ask the
parent node is if this new flag is set.

With this fix, your scenario would also no longer trigger a layout loop,
assuming the node you are modifying is a direct child of the node on
which layoutChildren is overridden.

The only reason that currently this broken logic is not already
triggering infinite layouts is that a 2nd pass will set all values to
the same values, and Node will not trigger its logic if the value was
unchanged.  However, its already broken that a 2nd pass is necessary at
all.  In effect, JavaFX is currently almost always doing **two** layout
passes; one that changes all the positions and sizes, and another that
"confirms" these unnecessarily.

As it is always dangerous to play with the layout logic, this will need
to be well tested, but I think we really need to fix this, or revert the
JDK-8360940 change as I think it may otherwise cause too much problems,
even though it is fixing problems and just exposing new ones...

--John



On 21/10/2025 21:42, Johan Vos wrote:
> I recently noticed a performance degradation in a JavaFX application,
> that I could trace to the fix for JDK-8360940 [1]. I believe the fix
> is correct, but it can cause infinite pulses in some cases. It is
> somehow discussed in the second of the 2 errors in Parent, in [2], but
> I believe this pattern is used often, and will now lead to infinite pulses
>
> For example, a class extending Parent and implementing layoutChildren
> that does something like this:
>
> @Override
> protected void layoutChildren() {
>     super.layoutChildren();
>     if (someSpecificCondition) {
>         someManagedNode.relocate(someX, someY);
>     }
> }
>
> Support the someManagedNode is at (x0, y) before the pulse runs.
> The call to super.layoutChildren() will set the layoutX/Y to e.g.
> superX, superY and it will request a new pulse (in case superX != x0
> or superY != y).
> Next, the someManagedNode is conditionally repositioned to someX, someY .
> In the next pulse (immediately following this pulse), the
> super.layoutChildren will again set the layoutX/Y to superX, superY,
> and trigger a new pulse, no matter what happens afterwards. The
> conditional code will again reset the layoutX/Y to someX, someY (which
> is what they had before this pulse started layoutChildren()), but the
> next pulse is scheduled AND it will visit this Node.
>
> Even though in the end the layoutX/layoutY properties of
> someManagedNode are not changed as the result of layoutChildren, a new
> pulse will be requested because there was an intermediate value (which
> never got rendered, as it was changed again even before the
> layoutChildren returns) that triggered the requestNextPulse.
>
> I think this pattern is used in a number of external controls. It
> worked "by accident" until the issue in JDK-8360940 was fixed. In
> defense of the pattern above, one could argue that a requestPulse
> should only be triggered if the triggering property was altered at the
> end of the layoutPhase (that is, if there is a difference in value
> between the start and the end of the layoutPhase).
>
> In summary, I don't propose to change anything, and I agree with the
> commit in [1], but it might need some attention in the release notes
> or docs.
>
> - Johan
>
>
> [1] https://github.com/openjdk/jfx/commit/5682424bebae7947f665fcb11429c3d2e069e8e5
> [2] https://bugs.openjdk.org/browse/JDK-8360940?focusedId=14811022
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20251023/43264a64/attachment.htm>