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

[John Hendrikx]

The code in Node that schedules a new pulse for changes to LayoutX/Y is
iffy at best IMHO.  It has a lot of guards and checks, most of which
don't really work properly.  It for example checks if the layoutX/Y is
being modified for the current layout child, but aside from
Parent#layoutChildren itself (which rarely does actual layout as its
code is overridden by real layout containers), no layout container
actually sets the current layout child before modifying the layout X/Y
of such a child.  This check therefore is effectively useless, but a
casual observer may conclude that this will ensure that a container
modifying layout X/Y will not schedule another layout (it does, but if
the calculation ends up the same, no 3rd layout is scheduled).

I'm not even entirely sure why Node bothers to do this check during a
layout pulse; position changes are expected during a layout pulse, and
if code is modifying these positions incorrectly or contrary to what the
layout container does for a managed node, then IMHO Node shouldn't
bother trying to correct this.  Your example should simply not trigger
another pulse at all, and the modification of a managed node's position
should just be accepted.  If it ends up visually in a bad location, then
that's on this bad code changing a **managed** node.

The Layout X/Y check code seems to be intended to "correct" positions of
a managed node when the change is made outside a layout pulse, although
I have a hard time imagining when that could be (a user calling
"relocate" casually on a managed node?).  It could be via bindings (even
though it is terrible practice to bind layout properties) but then those
changes would likely propagate during a layout pulse still.

Anyway, there may still be a way to alleviate this situation.  Instead
of Node immediately scheduling a new layout, it perhaps could hook into
a "layout finished" event when it detects a Layout X/Y change.  Once
layout completes, this hook runs, checks the original X/Y versus the
latest X/Y, and only triggers another layout if they differ.  It should
be effectively the same, as the layout scheduling is already a deferred
action.  This can probably be done with a Platform.runLater() or a pulse
listener.

Advantages of doing the above suggested modification could be:
- Reduction of unnecessary 2nd layout passes that change nothing (but
still run a ton of complex container calculations)
- Removal of "current layout child" checks, and the associated code in
Parent (it's practically useless code anyway)
- Probably kills the need for a "force parent layout" flag in Parent as
Node can just call regular "requestLayout" as it does so now after a
layout pass completes instead of during one

The more I think of it, the more I think that this would be a far better
solution.  It however does modify core layout code, and although I think
this could be a relatively safe change, I can't guarantee that code that
has been "working around" layout issues and relying on unspecified
semantics won't be affected (just like I didn't foresee the problem you
mentioned).

--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