On Thu, Oct 23, 2025 at 6:00 PM Kevin Rushforth <[email protected]> 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 <[email protected]> > 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 >> >> >
