Performance optimizations - #46
Open
jdolan wants to merge 19 commits into
Open
Conversation
Renders a representative game HUD (health, armor, ammo, crosshair, countdown timer, chat log, toggling scoreboard) with each widget updating on an interval, and times the style, layout, draw and endFrame passes individually, printing a summary once per second. Intended to measure CPU cost of driving a per-frame HUD with MVC before and after performance changes. MVC_HUD_FRAMES=N exits after N frames; MVC_HUD_HIDDEN=1 creates the window hidden. Baseline (M-series macOS, MVC_HUD_FRAMES=1400 MVC_HUD_HIDDEN=1, vsync ~120fps): idle frames style avg ~2-5us (max 233us), layout avg ~0.5us (max 47us), draw avg ~8-38us (max 2.5ms), endFrame avg ~25-52us (max 226us); 6 draw calls idle, 26 with scoreboard shown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduce propagating setters for the dirty flags: in addition to setting the flag on the View, they mark needsLayoutSubviews or needsApplyThemeSubviews on each ancestor, recording that a descendant is dirty. The ancestor walk stops at the first already-marked View, so repeated invalidations are amortized O(1). Convert every in-tree flag write to the setters (View internals and all widgets). No behavior change yet: the subtree flags are not read until the traversal gating that follows. Applications that assign needsLayout or needsApplyTheme directly MUST migrate to the setters; direct writes will not propagate, and the View may be skipped once applyThemeIfNeeded and layoutIfNeeded are gated on the subtree flags. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both traversals previously recursed into every subview each frame, making the per-frame cost O(tree) even when nothing was invalidated. They now return immediately unless the View or a descendant is dirty, making the steady-state cost O(dirty path). The subtree flag is cleared before doing any work, so invalidations that occur during the traversal itself (e.g. View::resize propagating setNeedsLayout, or a widget marking a sibling mid-layout) survive to the next frame rather than being lost. Behavior change: a dirty View's subviews are no longer laid out before the View itself. Previously layoutIfNeeded recursed into children first and then re-arranged them via layoutWithConstraint, laying out dirty children twice; children are now laid out once, by their parent's layout pass, with a follow-up recursion catching any descendant skipped by an overridden layoutSubviews. Examples/HUD idle frames: style pass avg 2-5us -> 0.2us; layout pass avg similar with maxima reduced. All tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both were recomputed from scratch on every call -- renderFrame is O(depth), and clippingFrame recursed into every clipping ancestor's clippingFrame, making it super-linear -- and they are called three to six times per View per frame (Renderer::drawView, View::render, subclass render methods), plus twice per View per mouse motion event. Views now memoize both rects, stamped against a process-global render frame generation. WindowController::renderTo bumps the generation via MVC_InvalidateRenderFrames after layout and before drawing, so the draw pass and subsequent hit-testing observe post-layout frames at O(1) amortized per View. A zero generation (callers that never invalidate, e.g. unit tests) disables cache reads entirely. clippingFrame now intersects only the nearest clipping ancestor's clippingFrame, which already folds in every outer clip; this is equivalent to the previous every-ancestor loop without the redundant super-linear work. Frames mutated outside of layout (e.g. Panel dragging) are observed by hit-testing at the next render rather than immediately; MVC_InvalidateRenderFrames is exported for callers that need same-batch precision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Text measurement went through Font::sizeCharacters on every call, which strdups the string and invokes SDL_ttf per line; a container measuring its children during layout re-measured every unchanged Text descendant. The color escapes path is far more expensive still. Cache the measured size on the Text, keyed by the Font's scale so pixel-density changes re-measure without additional hooks, and invalidate wherever the rendered texture is invalidated (setText, setFont, color change, scale change, device reset) as well as in awakeWithDictionary, whose text inlet bypasses setText. Examples/HUD layout-pass maxima on widget-update frames drop from ~25-50us to ~5-10us. All tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pushDrawArrays now merges a record into the previous one when both bind the same texture and scissor: vertices are appended contiguously, so extending the prior record's vertexCount is equivalent and saves a setScissor, bindFragmentSamplers and drawPrimitives per merged record in endFrame. Blending order is preserved since only adjacent records merge. Vertices are appended with one capacity check and a direct array write instead of a virtual Vector::add per vertex, and drawLines uses a stack buffer for polylines up to 16 segments (drawLine and drawRect always qualify) instead of a malloc/free per call -- previously every bordered View allocated every frame. Merging favors untextured geometry (backgrounds, borders, bevels share the 1x1 white texture); distinct Text textures still cost one draw each. All tests pass; Examples/HUD and Hello render identically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collect each cache's value and validity into a single anonymous struct member (renderFrameCache, clippingFrameCache on View; naturalSizeCache on Text) rather than parallel loose fields, keeping the value and the stamp or flag that guards it visibly paired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review found the self-first ordering introduced with traversal gating regressed convergence: a descendant whose layout resizes it marks its ancestors mid-pass, and with self-first ordering a clean ancestor's dirty check has already run, deferring its re-arrangement by one rendered frame per nesting level. Subviews-first restores the original bottom-up single-pass convergence while keeping the gating. Also complete setter adoption flagged by review: invalidateStyle's enumerator now uses setNeedsApplyTheme rather than writing the flag directly (the trailing self call becomes redundant), the layout unit tests use setNeedsLayout, and ScrollBar's docs reference the setter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
colorEscapes is a public, setter-less field that switches the measurement path, and flipping it after the first measurement (the documented usage) left the cache returning the escape-blind size indefinitely. Include it in the cache key. Addresses code review feedback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code review found the in-tree violators of the new invalidation contract: Panel mutates its frame directly while dragging, leaving hit-testing for the remainder of the event batch on the pre-move cached clippingFrame, and Text resizes itself mid-draw on a pixel density change but then read the renderFrame stamped earlier in the same pass. Both now call MVC_InvalidateRenderFrames after mutating. The HUD benchmark also never bumped the generation, so it measured the frame-cache feature disabled; it now mirrors renderTo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
progress and setValue computed value / (max - min), which reports 100% at zero progress for any non-zero min, and divides by zero when max == min (reachable via bound inlets with no validation). Compute (value - min) / (max - min), guarded to 0% when max <= min, and derive setValue's fraction from progress rather than duplicating the formula. Pre-existing defect surfaced by code review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors the ObjectivelyMVC-Hello command line tool target: HUD.c, the same framework links and search paths, and a shared scheme, so the benchmark can be run and profiled from Xcode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Multiplies the scoreboard row count so frame cost can be measured as a function of UI complexity; the default HUD is too small for tree-size-dependent costs to dominate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The UI passes accounted for only a fraction of the process's CPU; the acquire (RenderDevice::beginFrame through the clear pass) and submit (RenderDevice::endFrame, which blocks on present with vsync) columns attribute the remainder, distinguishing engine/driver floor from MVC cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jdolan
commented
Sep 2, 2026
Uh oh!
There was an error while loading. Please reload this page.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new Examples/HUD.c uses printf but does not include <stdio.h>, which can break builds under common C warning/error settings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request adds a new HUD example application intended for benchmarking, while also introducing render-frame caching/invalidation and several layout/theme invalidation improvements to reduce redundant work during rendering.
Changes:
- Added a new
HUDexample app and integrated it into both Automake and Xcode (new scheme/target). - Introduced per-frame render/clipping frame caching with an explicit
MVC_InvalidateRenderFrames()invalidation point after layout (and for direct frame mutation paths). - Optimized renderer batching (stack allocation for small line buffers; contiguous vertex appends and draw-call coalescing) and tightened layout invalidation via
View::setNeedsLayout/View::setNeedsApplyTheme.
File summaries
| File | Description |
|---|---|
| Tests/ObjectivelyMVC/View.c | Updates tests to use View::setNeedsLayout instead of direct flag writes. |
| Sources/ObjectivelyMVC/WindowController.c | Invalidates render-frame caches after layout, before draw. |
| Sources/ObjectivelyMVC/View.h | Adds cached frame fields + subtree invalidation flags; exports MVC_InvalidateRenderFrames(). |
| Sources/ObjectivelyMVC/View.c | Implements caching/invalidation generation, subtree invalidation propagation, and faster clipping/render frame computation. |
| Sources/ObjectivelyMVC/TextView.c | Switches to setNeedsLayout on edit/text changes. |
| Sources/ObjectivelyMVC/Text.h | Adds naturalSize cache state to Text. |
| Sources/ObjectivelyMVC/Text.c | Implements/invalidates naturalSize caching; invalidates render frames on device-reset resize mid-draw. |
| Sources/ObjectivelyMVC/TabView.c | Uses setNeedsLayout on tab selection changes. |
| Sources/ObjectivelyMVC/TableView.c | Replaces direct needsLayout writes with setNeedsLayout during layout/reload. |
| Sources/ObjectivelyMVC/Slider.c | Uses setNeedsLayout when value changes. |
| Sources/ObjectivelyMVC/Select.c | Uses setNeedsLayout after option mutations/selection. |
| Sources/ObjectivelyMVC/ScrollView.c | Uses setNeedsLayout for layout-affecting state changes. |
| Sources/ObjectivelyMVC/ScrollBar.h | Updates docs to reflect View::setNeedsLayout usage. |
| Sources/ObjectivelyMVC/ScrollBar.c | Uses setNeedsLayout after scroll interactions/state changes. |
| Sources/ObjectivelyMVC/Renderer.c | Reduces allocations for small line draws; appends vertices contiguously and coalesces compatible draw records. |
| Sources/ObjectivelyMVC/ProgressBar.c | Fixes progress calculation edge cases and uses setNeedsLayout. |
| Sources/ObjectivelyMVC/Panel.c | Invalidates render frames after direct frame mutation to keep hit-testing accurate within the event batch. |
| Sources/ObjectivelyMVC/PageView.c | Uses setNeedsLayout when current page changes. |
| Sources/ObjectivelyMVC/Option.c | Uses setNeedsLayout when selection state changes. |
| Sources/ObjectivelyMVC/Label.c | Uses setNeedsLayout after dictionary binding. |
| Sources/ObjectivelyMVC/Control.c | Uses setNeedsLayout on state changes. |
| Sources/ObjectivelyMVC/CollectionView.c | Uses setNeedsLayout after reload. |
| ObjectivelyMVC.xcodeproj/xcshareddata/xcschemes/ObjectivelyMVC-HUD.xcscheme | Adds an Xcode shared scheme for the new HUD example. |
| ObjectivelyMVC.xcodeproj/project.pbxproj | Adds the HUD target, source, and framework link settings to the Xcode project. |
| Examples/Makefile.am | Adds HUD to Automake example programs and defines HUD_SOURCES. |
| Examples/HUD.c | Introduces the HUD benchmark app (builds view tree, runs timed frame passes, prints periodic stats). |
| Examples/.gitignore | Ignores the new HUD example binary. |
Review details
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Replace the stack-or-malloc dual path with a fixed stack buffer, emitting long polylines in batches: pushDrawArrays merges adjacent records with equal texture and scissor, so a polyline of any length still produces a single draw call, with no heap allocation for any caller. In-tree callers pass at most four segments and take one batch. Addresses review feedback on #46. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 freeto 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.
This pull request introduces a new
HUDexample application to the project, updates the build system and Xcode project files to support it, and includes several code improvements and bug fixes in the core library. The most significant changes are grouped below.New Example Application: HUD
HUD.csource file and integrated a newHUDexample application into the build system (Makefile.am) and Xcode project (ObjectivelyMVC.xcodeproj), including a dedicated build scheme (ObjectivelyMVC-HUD.xcscheme). [1][2][3][4][5][6][7][8][9][10][11][12][13][14][15][16]Core Library Improvements and Bug Fixes
needsLayoutproperty with calls to thesetNeedsLayoutmethod for better encapsulation and consistency in layout invalidation across multiple components (CollectionView,Control,Label,Option,PageView,ProgressBar). [1][2][3][4][5][6]ProgressBarvalue calculation to handle edge cases wheremax <= min, preventing divide-by-zero and ensuring correct progress display.Panel, after direct frame mutation, added a call toMVC_InvalidateRenderFrames()to ensure hit-testing uses up-to-date frame data.Renderer Performance Optimization
Renderer::drawLinesto use stack allocation for small vertex buffers, reducing heap allocations and improving rendering performance. [1][2]