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

[Johan Vos]

On Thu, Oct 23, 2025 at 6:00 PM Kevin Rushforth <kevin.rushforth at oracle.com>
wrote:

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

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.


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

Absolutely.

- Johan


>
> -- Kevin
>
> On 10/23/2025 12:16 AM, 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/20251024/43bb1abb/attachment-0001.htm>