Perf/scoped UI clean and styles#774
Open
Colorfingers wants to merge 2 commits into
Open
Conversation
Three coupled changes that stop one event from re-laying-out the whole document. Pure perf — no UX or API removed. ui.Clean now walks only the receiver's subtree instead of always walking from rootUI. Cross-element layout, scissor, and render have no sibling dependencies (children read only their own layout and ancestor scissors, both unchanged when cleaning a subtree), so a subtree clean produces correct output without touching unrelated panels. The per-frame Manager.update path still calls Clean on root UIs, so root.Clean still covers the whole tree when needed. The unused rootUI helper is removed. cleanIfNeeded now walks down to the topmost dirty element on each branch and cleans only that subtree, instead of force-cleaning from root whenever any descendant is dirty. setDirtyInternal cascades dirty downward but never upward, so the topmost dirty element on each branch covers every dirty element exactly once. Stylizer.ApplyStylesToElement is added (clears + re-applies CSS rules only inside target's subtree) and the Doc.* mutators that previously re-styled the whole document now scope to the affected element: SetElementId, SetElementClasses, ChangeElementParent, DuplicateElement, DuplicateElementToParent, DuplicateElementRepeat (and the WithoutApplyStyles variant that despite its name was applying), and insertElementAt. Doc.ApplyStyles, the document-load setup, the window-resize handler, and RemoveElement still use the doc-wide path since those are genuinely document-affecting or one-shot. Caveat: scoped ApplyStylesToElement is correct under the engine's current selector grammar (id, class, tag, pseudo, attribute condition, descendant). If sibling combinators (`+`, `~`) are added later, the scope must widen accordingly. Discovered while profiling drag-drop in a heavily-populated editor: dropping a model was triggering 18 separate clean calls covering ~1500 elements across multiple roots, because every Doc.* mutator marked every element in the document dirty. With these three changes a drop cleans only the affected subtrees.
Keeps Claude Code's local working directory out of the repo.
Contributor
Author
|
I'm realizing this messes up the formatting of the Settings links... use with caution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Kaiju PR: Scope UI clean and CSS re-application to subtree
Branch:
perf/scoped-ui-clean-and-styles(off latestmaster)Commit:
311dd482— "Scope UI clean and CSS re-application to subtree"Files changed: 4 (+127 / -38)
The bug, plainly
Kaiju's UI engine has a single design choice that becomes a problem at scale: any change anywhere in a document re-lays-out the entire document. That's not three different bugs — it's one architecture decision implemented in three places, and you have to fix all three for any of them to matter.
The three changes, and why each one matters
1.
UI.Clean()was global, not localUI.Clean()reads like a method on one element ("clean this thing"). It isn't. The first thing it did wasroot := ui.rootUI()— walk up to the document root — then iterate every active element in the entire document, re-running layout, scissor, and render on each.So
someElement.Clean()anddocumentRoot.Clean()did the exact same work. The receiver was decoration.Why fixing this is safe: I audited the layout/scissor/render pipeline for cross-element reads.
There are no horizontal dependencies. So a subtree clean produces correct output for the subtree without touching unrelated panels.
The fix is one line:
root := uiinstead ofroot := ui.rootUI(). The unusedrootUI()helper is removed.What's preserved: the per-frame
Manager.updateloop calls Clean on root UIs, androot.Clean()still walks the whole tree. So whole-doc cleans still happen when they should.2.
cleanIfNeeded()defeated #1cleanIfNeeded()is what the per-frame loop calls. It checkedanyChildDirty()— recursive: "is anything anywhere below me dirty?" — and if yes, calledClean()on root.So even if I fixed #1 (subtree-scoped Clean), the per-frame path was still doing root.Clean every time anything was dirty. Same end result.
setDirtyInternal(the function that marks things dirty) cascades dirty downward to children but never upward to parents. So the topmost dirty element on any branch is exactly the element that originally became dirty (or its ancestor, if the change was at the root). Cleaning from there covers every dirty element exactly once.The fix: walk down from root. At the first dirty element on each branch, Clean that subtree and stop descending. Subtrees with no dirty elements are never visited.
3.
Stylizer.ApplyStylesre-styles the whole documentCSS rule application happens in two places: when the document loads (one-shot, fine) and whenever an element's id, class, parent, or position in the tree changes (frequent, hot path).
The hot-path methods —
SetElementId,SetElementClasses,ChangeElementParent,DuplicateElement,DuplicateElementToParent,DuplicateElementRepeat,insertElementAt— all calledDoc.ApplyStyles(), which walked every element in the document and re-evaluated every CSS rule against it. Each rule's processor (SetMargin,SetPadding,SetBackground, etc.) then calledSetDirtyon its target element.Net effect: changing one element's class marked every element in the document dirty. Combined with the unscoped Clean, the next frame re-laid-out everything.
Even with fixes #1 and #2, this layer would keep dirtying the whole document and forcing whole-doc cleans on the next frame. So you have to fix this too.
The new
Stylizer.ApplyStylesToElement(target)does the same selector-matching pass against the whole stylesheet (correct — selectors match wherever they match), but the clear and apply phases skip every element outsidetarget's subtree. So unrelated elements never see ClearRules, never get a SetDirty call, never become dirty.The hot-path mutators on
Documentswitch fromApplyStylestoApplyStylesToElement(elm).Doc.ApplyStyles(), the document-load setup, the window-resize handler, andRemoveElementkeep using the doc-wide path because they're either one-shot or genuinely document-affecting.Why all three are necessary
If you fix only #1: explicit
someElement.Clean()calls scope correctly, but per-framecleanIfNeededstill triggers whole-doc cleans whenever anything is dirty. No visible improvement during normal interaction.If you fix only #2: per-frame cleans are scoped, but every CSS mutation still marks the whole doc dirty (via #3), so every frame still has dirty subtrees everywhere, so cleans still cover everything.
If you fix only #3: dirty state is now scoped, but
cleanIfNeededstill walks from root via #2, andCleanstill walks the whole doc via #1. Everything still re-lays-out.The three changes are coupled: each one removes a different mechanism by which a single change cascades to the whole document. Together, a class change on one element re-styles only that element, marks only that subtree dirty, and the next frame's clean walks only that subtree. Apart, none of them have observable effect.
Caveat called out in the commit message
The scoped
ApplyStylesToElementis correct under Kaiju's current selector grammar (id, class, tag, pseudo, attribute condition, descendant). If sibling combinators (+,~) are added later, callers that previously relied on the doc-wide behavior will need to widen their scope — a sibling-targeted rule on element A would not re-evaluate when its sibling B mutates if we only re-style B's subtree.Today this is a non-issue because the selector matcher in
engine/ui/markup/css/reader.godoesn't parse those combinators. Worth noting in case it changes.The proof
Discovered while profiling drag-drop in a heavily-populated downstream editor: dropping a model on the stage was triggering 18 separate Clean calls covering ~1500 elements — basically the entire UI — every drop. Icons flickered, panels went blank momentarily. After all three changes, a drop cleans only the affected subtrees (the new entity's hierarchy row + the details panel). Nothing else is touched.
Was this a downstream-only bug?
Worth challenging directly: was this caused by the downstream's GLB / spawn code, or is it Kaiju's?
The GLB code path on drop is
dropContent→spawnContentAtMouse→spawnModel→ parse GLB, upload textures, attach mesh, push drawing. All of that is 3D scene work. None of it touches the UI document, marks UI elements dirty, or callsApplyStyles. If that were the only thing happening, the trace would have shown zero Clean calls.What actually drove the 18 Cleans, per the trace, was two trigger points — both of which are vanilla editor-UI code that exists in Kaiju too:
DetailsUI.entitySelected→reload()→dui.detailsArea.UI.Clean()(synchronous; line is in Kaiju'sstage_workspace_details_ui.go).HierarchyUI.entityCreatedandentitySelectedcallingDoc.DuplicateElement,Doc.SetElementId,Doc.SetElementClasses(each of which callsDoc.ApplyStyles()doc-wide).What the downstream did was amplify the symptom, not cause it:
The underlying mechanism —
Doc.ApplyStylesbeing doc-wide andcleanIfNeededwalking from root — is pure Kaiju, predates the downstream work, and would bite any consumer whose docs grew past a few hundred elements.Kaiju's editor today is small enough that the user might not notice this overhead, which is why the design has survived. But it's the kind of thing that gets worse as docs grow, and the fix is local and safe enough that it's worth doing now rather than after someone hits the wall.
Files in this PR
src/engine/ui/ui.goUI.Clean; top-most-dirtycleanIfNeeded; removed unusedrootUIandanyChildDirtyhelpers.src/engine/ui/markup/document/html_style_interfaces.goApplyStylesToElementto theStylizerinterface.src/engine/ui/markup/css/reader.goStylizer.ApplyStylesToElement.src/engine/ui/markup/document/html_parser.goDoc.ApplyStylesToElement; switchedSetElementId/SetElementClasses/ChangeElementParent/DuplicateElement/DuplicateElementToParent/DuplicateElementRepeat(and itsWithoutApplyStylesvariant) /insertElementAtto scoped variant.No public API removed. One method added to a public interface (
Stylizer.ApplyStylesToElement) — any external Stylizer implementations will need to implement it.