<div dir="ltr"><div dir="ltr">On Thu, Oct 23, 2025 at 6:00 PM Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com">kevin.rushforth@oracle.com</a>> wrote:</div><div class="gmail_quote gmail_quote_container"><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>
Regarding the specification, are you primarily thinking about the
need to document the implementation of layout or do you think
additional documentation for the public / protected layout methods
(e.g., layoutChildren) or class docs would be useful?<br></div></blockquote><div><br></div><div>I believe the highest priority should go to documentation for openjfx developers and reviewers. I agree that it would be good to have docs/guidelines for JavaFX developers, but I think the primary concern is a deeper knowledge and consistency for our internal development. Most of the (implicit) specification is in code and different levels of documentation inside the java files. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<div></div>
Regarding testing, I can see the need for additional tests. In most
cases, layout tests can be done using unit tests in the
javafx.graphics module. These tests use the StubToolkit (instead of
the actual QuantumToolkit), so glass and prism are not even loaded.<br>
<br>
Unrelated to layout or any particular test, switching the
javafx.graphics and javafx.controls test from StubToolkit to
QuantumToolkit + headless glass would be an interesting thing to
look at. This might a a good discussion to split into its own
thread. I can definitely see some value in doing this.<br></div></blockquote><div><br></div><div>Absolutely.</div><div><br></div><div>- Johan</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
<br>
-- Kevin<br>
<br>
<div>On 10/23/2025 12:16 AM, Johan Vos
wrote:<br>
</div>
<blockquote type="cite">
<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">
<div dir="ltr" class="gmail_attr">On Thu, Oct 23, 2025 at
2:49 AM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank">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">
<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>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>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>
</blockquote>
<br>
</div>
</blockquote></div></div>