performance impact of JDK-8360940 in case of a wrong but common pattern
John Hendrikx
john.hendrikx at gmail.com
Thu Oct 23 13:03:02 UTC 2025
I did a simple experiment. I implemented the new boolean flag (I called
it `inLayoutChildren`), and have Node check that instead of checking
`isCurrentLayoutChild` which is broken.
Everything looks okay in a fairly complex application.
For non-local UI changes (when the parent box also needs to change
size), FX normally always did 2 layouts. This becomes now one layout
pass as one would expect. I don't notice any adverse effects here
(perhaps it feels a bit snappier now).
For local UI changes (when a control changes size, but does not affect
its parent size since it had "unused" space), FX normally does 1 layout,
and this has stayed the same.
The change is very simple and minimal, and seems to make much more
sense. I think I'll create a PR for this so everyone can test and play
with it.
--John
On 23/10/2025 09:16, Johan Vos wrote:
> Hi Johan,
>
> 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.
> 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.
>
> 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.
>
> - Johan
>
> On Thu, Oct 23, 2025 at 2:49 AM John Hendrikx
> <john.hendrikx at gmail.com> wrote:
>
> 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/f25bf9f3/attachment-0001.htm>
More information about the openjfx-dev
mailing list