<div dir="ltr">Hi Johan,<div><br></div><div>I didn't look in detail yet (and checked with the code), but your analysis makes sense. I believe the root problem, though, is lack of documentation/specs. Our knowledge is mainly based on reverse engineering and reading the code. That allowed me to isolate and describe the "problem" I described in my original post, but even that is far from complete, as the layout process spans more classes and phases. I typically use pen and paper to draw the flow for isolating issues like this, but those pictures are never complete.</div><div>The second thing is, testing. Ideally, that should be related to specs, but in absence of specs we can test the current situation, and mark that as the lower bar. However, that implies that tons of tests would need to be added. And even then, there might be fixes that in general would lead to less pulses, but in corner cases might have a slight performance penalty.</div><div><br></div><div>In summary, I believe the specification part is the most important one. That will enable rigorous testing. With the headless platform in place now, those tests do not even require a real graphics pipeline.</div><div><br></div><div>- Johan</div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Thu, Oct 23, 2025 at 2:49 AM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com">john.hendrikx@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><u></u>
<div>
<p>Johan, I had a closer look.</p>
<p>TLDR: </p>
<p>- 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<br>
- I think it may be possible to fix this, and as a side effect
also fixes your infinite layout loop scenario <br>
</p>
<p>Longer version:<br>
</p>
<p>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.</p>
<p>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:</p>
<div style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px">
<div style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(63,127,95)">// Propagate layout if this change isn't triggered by its parent</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,160);font-weight:bold">if</span><span style="color:rgb(0,0,0)"> (p != </span><span style="color:rgb(0,0,160);font-weight:bold">null</span><span style="color:rgb(0,0,0)"> && !p.isCurrentLayoutChild(Node.</span><span style="color:rgb(0,0,160);font-weight:bold">this</span><span style="color:rgb(0,0,0)">)) {</span></p></div>
</div>
<p></p>
<p>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: </p>
<p> *** Is this layout X/Y change performed while the direct
Parent's layoutChildren() call is higher up in the call stack? ***
<br>
</p>
<p>There are two ways to answer that question:</p>
<p>- 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)<br>
- 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():<br>
</p>
<div style="background-color:rgb(255,255,255);padding:0px 0px 0px 2px">
<div style="color:rgb(0,0,0);background-color:rgb(255,255,255);font-family:Consolas;font-size:11pt;white-space:pre-wrap"><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,192)">performingLayout</span><span style="color:rgb(0,0,0)"> = </span><span style="color:rgb(0,0,160);font-weight:bold">true</span><span style="color:rgb(0,0,0)">;</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> layoutChildren();
... (more code that triggers layouts for the rest of the sub tree) ...
</span></p><p style="margin:0px"><span style="color:rgb(0,0,0)"> </span><span style="color:rgb(0,0,192)">performingLayout</span><span style="color:rgb(0,0,0)"> = </span><span style="color:rgb(0,0,160);font-weight:bold">false</span><span style="color:rgb(0,0,0)">;</span></p><span style="color:rgb(0,0,0)"></span><p style="margin:0px"><span style="color:rgb(0,0,0)">
</span></p></div>
</div>
<p></p>
<p>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.<br>
<br>
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.<br>
</p>
<p>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.<br>
</p>
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.<br>
<br>
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...
<p>--John<br>
</p>
<p><br>
</p>
<p><br>
</p>
<div>On 21/10/2025 21:42, Johan Vos wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>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</div>
<div><br>
</div>
<div>For example, a class extending Parent and implementing
layoutChildren that does something like this:</div>
<div><br>
</div>
@Override <br>
protected void layoutChildren() {
<div> super.layoutChildren();</div>
<div> if (someSpecificCondition) {</div>
<div> someManagedNode.relocate(someX, someY);</div>
<div> }</div>
<div>}<br>
<div><br>
</div>
<div>Support the someManagedNode is at (x0, y) before the
pulse runs.</div>
<div>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).</div>
<div>Next, the someManagedNode is conditionally repositioned
to someX, someY .</div>
<div>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.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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).</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>- Johan</div>
<div><br>
</div>
<div><br>
</div>
<div>[1] <a href="https://github.com/openjdk/jfx/commit/5682424bebae7947f665fcb11429c3d2e069e8e5" target="_blank">https://github.com/openjdk/jfx/commit/5682424bebae7947f665fcb11429c3d2e069e8e5</a></div>
</div>
<div>[2] <a href="https://bugs.openjdk.org/browse/JDK-8360940?focusedId=14811022" target="_blank">https://bugs.openjdk.org/browse/JDK-8360940?focusedId=14811022</a></div>
</div>
</blockquote>
</div>
</blockquote></div>