diff --git a/CHANGES.md b/CHANGES.md index 7e4be9b..a562e16 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,17 @@ # Changes +## Unreleased + +- Mark recursive fragment and diagnostic render trees acyclic; rely on Nim + inference for other render data. Document the downward ownership contract. +- Add cross-thread ARC/ORC render-data coverage and fragment-cycle rejection + regressions. +- Borrow the host window in the Siwin renderer and its OpenGL fallback state, + breaking the window/callback/renderer cycle. Callers keep the window alive + through rendering and renderer cleanup. +- Add `finishPendingFrames` to wait for submitted Metal, Vulkan, or OpenGL work + before releasing a presentation target. Renderer types remain cycle-capable. + ## 0.42.0 - Add immutable shared glyph arrangements and glyph-range views for line render diff --git a/README.md b/README.md index 491344e..8769fe8 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,9 @@ Features: - Fast Gaussian 2-pass node operation for fast background blurs. - Clipping and layering support. +See [render ownership](docs/render_ownership.md) for the acyclic render-data +contract and ARC/ORC thread-transfer requirements. + ## Quick Start This part works best with a recent Atlas (>= 0.9.6) version: diff --git a/docs/render_ownership.md b/docs/render_ownership.md new file mode 100644 index 0000000..4275b9c --- /dev/null +++ b/docs/render_ownership.md @@ -0,0 +1,85 @@ +# Render-data ownership + +FigDraw render data has an acyclic ownership contract under ARC and ORC. +Only the recursive `RenderFragment` and diagnostic `RenderTree` types need +explicit `{.acyclic.}` annotations. Nim already infers acyclicity for `Fig`, +`RenderList`, `Renders`, `RenderFragments`, and the glyph/text payload types. +The annotation records an ownership contract; it does not break cycles or make +mutable objects safe to share concurrently. + +| Type | Owning references and invariant | +| --- | --- | +| `Fig`, `RenderList`, `Renders` | Own drawing values and text payloads. `Fig.parent` is an integer index; nodes never retain their owner. | +| `RenderFragment` | Owns descendant fragments only. It has no parent or tree reference. | +| `RenderFragments` | Owns base lists and downward fragment edges. It never stores handles or cursors. | +| `RenderFragmentHandle`, `RenderCursor` | Retain the tree and fragment for safe invalidation checks. Nothing in that tree retains them. | +| `Utf8Runes`, `GlyphArrangement` | UTF-8 storage contains only bytes/checkpoints. Glyph views retain an immutable complete arrangement; that arrangement cannot itself be a view. | +| `GlyphPosition` | Contains only glyph geometry values. | +| `RenderTree` | Diagnostic output with downward child references. Callers must not insert the tree itself or an ancestor into its public `children` sequence. | + +`moveFragment` rejects both self-attachment and attachment beneath a descendant +before mutating the graph. Its attachment records contain a parent reference only +temporarily while moving an edge; fragments do not retain those records. + +Transfer exclusive ownership through an isolated channel payload and release +producer-side aliases before handoff. Acyclic annotations do not make reference +counts atomic. `FontRef` and `ImageRef` remain thread-affine managed leases; +transfer their IDs and establish ownership on the receiving thread instead. + +## Renderer ownership is separate + +This contract does not cover `FigRenderer`, backend contexts, or native windows. +An owning window reference formed a real cycle when a callback stored on that +window captured the renderer: + +```text +renderer -> backendState.window -> onRender closure -> renderer +``` + +The Siwin backend and its OpenGL fallback state now borrow their window using +`{.cursor.}`, removing the renderer-to-window owning edge. The caller must keep +the window alive through rendering and renderer cleanup. This is a borrow, not +a weak reference that automatically becomes nil when the window is destroyed. +Other window shims and custom backend states retain their existing contracts. + +`finishPendingFrames` waits for submitted GPU work on the renderer's owning +thread: Metal waits for its last submitted command buffer, Vulkan waits for the +device, and OpenGL finishes the current context. Call it after the final frame +and before releasing the renderer or its native presentation target. Custom +asynchronous backend contexts must override the operation. + +The renderer's `contextActivation` procedure uses `nimcall` and has no captured +closure environment. It does not introduce that cycle. + +Siwin's `useDedicatedRenderThread` already clears the window reference before a +handoff. However, clearing that reference and moving the object does not itself +remove an earlier registration in the creating thread's ORC cycle-candidate +buffer. After releasing producer-side aliases and assembling an exclusively +owned, isolated renderer payload, the sender must retire those registrations +with `GC_runOrc()` before publishing the payload to another thread. Sigils +`RChan` can do this with `-d:nimSafeOrcSend`; Merenda instead performs the +collection only for renderer commands, avoiding collections for each acyclic +frame snapshot. Render-data annotations alone do not fix renderer handoff. + +## Verification + +The ownership regression exercises sixteen exclusive transfers of previously +aliased fragment trees, render lists, diagnostic trees, and shared glyph views. +The fragment suite verifies that cycle-forming moves fail without changing +handles or topology. The Siwin callback regression verifies immediate context +destruction when the owning window leaves scope. + +```sh +atlas-run tests renderownership renderfragments siwin_dedicated_render -- --mm:orc +atlas-run tests renderownership renderfragments siwin_dedicated_render -- --mm:arc +``` + +For sanitizer coverage, use Clang with malloc-backed Nim allocation and pass the +sanitizers to both compilation and linking: + +```sh +UBSAN_OPTIONS=halt_on_error=1 atlas-run tests renderownership -- \ + --mm:orc --cc:clang -d:useMalloc -d:noSignalHandler --debugger:native \ + --passC:'-fsanitize=address,undefined -fno-omit-frame-pointer' \ + --passL:'-fsanitize=address,undefined' +``` diff --git a/figdraw.nimble b/figdraw.nimble index 2c2017f..b9f3144 100644 --- a/figdraw.nimble +++ b/figdraw.nimble @@ -1,4 +1,4 @@ -version = "0.42.0" +version = "0.43.0" author = "Jaremy Creechley" description = "UI Engine for Nim" license = "MIT" diff --git a/src/figdraw/common/fonttypes.nim b/src/figdraw/common/fonttypes.nim index f5033de..f2a7a08 100644 --- a/src/figdraw/common/fonttypes.nim +++ b/src/figdraw/common/fonttypes.nim @@ -121,6 +121,7 @@ type type GlyphArrangement* = object + ## Views retain immutable complete layouts; owners never retain their views. shared*: ConstPtr[GlyphArrangement] ## Immutable owner for a glyph-range view. viewGlyphStart*: uint32 ## Inclusive glyph offset in `shared`. viewGlyphEnd*: uint32 ## Exclusive glyph offset in `shared`. diff --git a/src/figdraw/common/transfer.nim b/src/figdraw/common/transfer.nim index 3b4c074..5ba6152 100644 --- a/src/figdraw/common/transfer.nim +++ b/src/figdraw/common/transfer.nim @@ -1,7 +1,9 @@ import std/[math, sequtils] import ../fignodes -type RenderTree* = ref object +type RenderTree* {.acyclic.} = ref object + ## Diagnostic tree with downward-only child ownership. Callers must not insert + ## the tree itself or an ancestor into `children`. id*: int children*: seq[RenderTree] diff --git a/src/figdraw/figbackend.nim b/src/figdraw/figbackend.nim index 960413a..438dfc8 100644 --- a/src/figdraw/figbackend.nim +++ b/src/figdraw/figbackend.nim @@ -633,6 +633,12 @@ method beginFrame*( method endFrame*(impl: BackendContext) {.base.} = raise newException(ValueError, "Backend endFrame unavailable") +method finishPendingFrames*(impl: BackendContext) {.base.} = + ## Waits for submitted GPU work before a presentation target is released. + ## Call on the owning render thread, after ending the last frame. Backends + ## that submit asynchronous work must override this operation. + discard + method translate*(impl: BackendContext, v: Vec2) {.base.} = raise newException(ValueError, "Backend translate unavailable") diff --git a/src/figdraw/fignodes.nim b/src/figdraw/fignodes.nim index b6a12de..f1cf899 100644 --- a/src/figdraw/fignodes.nim +++ b/src/figdraw/fignodes.nim @@ -43,11 +43,13 @@ type RenderShadows* = array[ShadowCount, RenderShadow] Renders* = ref object + ## Owns value-only render lists. Nodes must never retain their render owner. layers*: OrderedTable[ZLevel, RenderList] FigIdx* = distinct int16 Fig* = object + ## Render payloads are acyclic. Parent links are indexes, not owning refs. zlevel*: ZLevel parent*: FigIdx = (-1).FigIdx flags*: set[FigFlags] diff --git a/src/figdraw/figrender.nim b/src/figdraw/figrender.nim index a442bda..f8c7bfe 100644 --- a/src/figdraw/figrender.nim +++ b/src/figdraw/figrender.nim @@ -75,6 +75,12 @@ proc activateContext*[BackendState](renderer: FigRenderer[BackendState]) = if not renderer.contextActivation.isNil and renderer.backendKind() == rbOpenGL: renderer.contextActivation(renderer) +proc finishPendingFrames*[BackendState](renderer: FigRenderer[BackendState]) = + ## Waits for submitted frames on the renderer's owning thread. The host window + ## must remain alive until this returns and renderer cleanup has finished. + renderer.activateContext() + renderer.ctx.finishPendingFrames() + proc atlasUsage*[BackendState](renderer: FigRenderer[BackendState]): AtlasUsage = ## Returns current backend atlas usage. ## diff --git a/src/figdraw/metal/metal_context.nim b/src/figdraw/metal/metal_context.nim index ed323d6..a4c5028 100644 --- a/src/figdraw/metal/metal_context.nim +++ b/src/figdraw/metal/metal_context.nim @@ -1901,6 +1901,12 @@ method endFrame*(ctx: MetalContext) = ctx.commandBuffer.clear() ctx.frameAutoreleasePool.stop() +method finishPendingFrames*(ctx: MetalContext) = + # All submissions use one command queue, so its last buffer covers earlier + # frames too. Waiting must precede destruction of the presentation layer. + if not ctx.lastCommitted.isNil: + waitUntilCompleted(ctx.lastCommitted.borrow) + method translate*(ctx: MetalContext, v: Vec2) = ctx.mat = ctx.mat * translate(vec3(v)) diff --git a/src/figdraw/opengl/glcontext.nim b/src/figdraw/opengl/glcontext.nim index d419fcf..0b58616 100644 --- a/src/figdraw/opengl/glcontext.nim +++ b/src/figdraw/opengl/glcontext.nim @@ -2055,6 +2055,9 @@ proc beginFrameDefaultProj(ctx: OpenGlContext, frameSize: Vec2) = ctx, frameSize, ortho[float32](0.0, frameSize.x, frameSize.y, 0, -1000.0, 1000.0) ) +method finishPendingFrames*(ctx: OpenGlContext) = + glFinish() + method endFrame*(ctx: OpenGlContext) = ## Ends a frame. assert ctx.frameBegun == true, "ctx.beginFrame was not called first." diff --git a/src/figdraw/renderfragments.nim b/src/figdraw/renderfragments.nim index ddc8c63..9fc80af 100644 --- a/src/figdraw/renderfragments.nim +++ b/src/figdraw/renderfragments.nim @@ -107,7 +107,9 @@ type parent: FigIdx position: int - RenderFragment = ref object + RenderFragment {.acyclic.} = ref object + # Owns descendant fragments only. Parent attachments are temporary values; + # moveFragment rejects attachment beneath this fragment or a descendant. id: uint64 list: RenderList entries: RenderEntries @@ -121,6 +123,8 @@ type ## ## Fragment edges are stored separately from physical `RenderList` nodes, so ## replacement does not change indexes in the surrounding lists. + ## Owning edges point downwards. Handles/cursors retain this tree, but the + ## tree never retains handles/cursors or parent links. base: Renders layerEntries: OrderedTable[ZLevel, RenderEntries] layerGenerations: Table[ZLevel, uint64] @@ -1284,7 +1288,7 @@ proc appendMaterializedEntry( owner: RenderFragment, entry: RenderChild, parent: FigIdx, -) +) {.gcsafe.} proc appendMaterializedNode( fragments: RenderFragments, diff --git a/src/figdraw/vulkan/vulkan_context.nim b/src/figdraw/vulkan/vulkan_context.nim index a39711d..b99a05a 100644 --- a/src/figdraw/vulkan/vulkan_context.nim +++ b/src/figdraw/vulkan/vulkan_context.nim @@ -3757,6 +3757,10 @@ method beginFrame*( clearMainColor = clearMainColor, ) +method finishPendingFrames*(ctx: VulkanContext) = + if ctx.device != vkNullDevice: + checkVkResult ctx.vk.vkDeviceWaitIdle(ctx.device) + method endFrame*(ctx: VulkanContext) = assert ctx.frameBegun == true, "ctx.beginFrame was not called first." assert ctx.maskDepth == 0, "Not all masks have been popped." diff --git a/src/figdraw/windowing/siwinshim.nim b/src/figdraw/windowing/siwinshim.nim index d43f119..fbdb681 100644 --- a/src/figdraw/windowing/siwinshim.nim +++ b/src/figdraw/windowing/siwinshim.nim @@ -463,7 +463,7 @@ when UseVulkanBackend and UseOpenGlFallback and (defined(linux) or defined(bsd)) SiwinOpenGlFallbackState = ref SiwinOpenGlFallbackStateObj SiwinOpenGlFallbackStateObj = object - window: Window + window {.cursor.}: Window initialized: bool case kind: SiwinOpenGlFallbackKind of sogfX11: @@ -490,15 +490,13 @@ when UseVulkanBackend and UseOpenGlFallback and (defined(linux) or defined(bsd)) state.waylandContext.destroy() except Exception: discard - try: - `=destroy`(state.window) - except Exception: - discard type SiwinRenderBackend* = object ## Opaque per-window backend state used by siwin + FigDraw integration. - window*: Window + window* {.cursor.}: Window + ## Borrowed from the caller, which must keep the window alive while using + ## the renderer. Window callbacks may therefore own the renderer safely. dedicatedRender*: bool presentationReady: bool resizeClearColor: Color @@ -787,6 +785,8 @@ proc activateRendererContext(renderer: FigRenderer[SiwinRenderBackend]) = proc setupBackend*(renderer: FigRenderer, window: Window) = ## One-time backend hookup between a siwin window and FigDraw renderer. + ## The caller owns `window` and keeps it alive until rendering and renderer + ## cleanup finish. Dedicated rendering clears this borrow before transfer. renderer.backendState.window = window renderer.contextActivation = activateRendererContext renderer.backendState.dedicatedRender = false diff --git a/tests/trenderfragments.nim b/tests/trenderfragments.nim index 491e36a..eb5a30b 100644 --- a/tests/trenderfragments.nim +++ b/tests/trenderfragments.nim @@ -362,6 +362,26 @@ suite "RenderFragments APIs": check reorderedRoot == first check fragments.rootCursors(0.ZLevel).mapIt(fragments[it].nodeId()) == @[10, 20] + test "self and descendant attachment cannot create an ownership cycle": + let fragments = newRenderFragments() + let root = fragments.addRoot(0.ZLevel, testFig(10)) + var parentContents = RenderList() + discard parentContents.addRoot(testFig(20)) + let parent = fragments.attachChildFragment(0.ZLevel, root, 0, move parentContents) + let parentRoot = fragments.fragmentRoots(parent)[0] + var childContents = RenderList() + discard childContents.addRoot(testFig(30)) + let child = fragments.attachChildFragment(parentRoot, 0, move childContents) + let childRoot = fragments.fragmentRoots(child)[0] + expect RenderFragmentError: + discard fragments.moveFragment(parent, parentRoot, 0) + expect RenderFragmentError: + discard fragments.moveFragment(parent, childRoot, 0) + check fragments.isValid(parent) + check fragments.isValid(child) + check fragments.childIds(parentRoot) == @[30] + check fragments.materialize()[0.ZLevel].nodes.len == 3 + test "moves an empty fragment slot before restoring its contents": let fragments = newRenderFragments() let root = fragments.addRoot(0.ZLevel, testFig(10)) diff --git a/tests/trenderownership.nim b/tests/trenderownership.nim new file mode 100644 index 0000000..c0850c9 --- /dev/null +++ b/tests/trenderownership.nim @@ -0,0 +1,71 @@ +import std/[isolation, unittest] +import threading/smartptrs +import sigils/rchannels + +import figdraw/common/[fonttypes, transfer] +import figdraw/fignodes +import figdraw/renderfragments + +type RenderTransfer = object + fragments: RenderFragments + renders: Renders + tree: RenderTree + +proc retainAlias[T](value: T): T {.noinline.} = + value + +proc makeTransfer(): RenderTransfer = + let source = initArrangementRunes("a🙂b") + let owner = shareGlyphArrangement( + GlyphArrangement( + sourceRunes: source, + runes: source, + arrangedGlyphs: @[ArrangedGlyph(), ArrangedGlyph(), ArrangedGlyph()], + ) + ) + result.fragments = newRenderFragments() + let root = result.fragments.addRoot(0.ZLevel, Fig(kind: nkRectangle)) + var contents = RenderList() + discard contents.addRoot(Fig(kind: nkRectangle)) + let fragment = result.fragments.attachChildFragment(0.ZLevel, root, 0, move contents) + let cursor = result.fragments.fragmentRoots(fragment)[0] + var nested = RenderList() + discard + nested.addRoot(Fig(kind: nkText, textLayout: owner.glyphArrangementView(1 .. 2))) + discard result.fragments.attachChildFragment(cursor, 0, move nested) + result.renders = result.fragments.materialize() + result.tree = result.renders[0.ZLevel].toTree() + + # Releasing aliases before handoff must not leave render data registered in + # the producer's ORC candidate buffer. All handles and cursors also expire + # before this procedure returns its exclusively owned payload. + var fragmentsAlias = retainAlias(result.fragments) + var rendersAlias = retainAlias(result.renders) + var treeAlias = retainAlias(result.tree) + doAssert not fragmentsAlias.isNil + doAssert not rendersAlias.isNil + doAssert not treeAlias.isNil + fragmentsAlias = nil + rendersAlias = nil + treeAlias = nil + +proc releaseOnWorker(channel: RChan[RenderTransfer]) {.thread.} = + let payload = channel.recv() + doAssert payload.fragments.materialize()[0.ZLevel].nodes.len == 3 + doAssert payload.renders[0.ZLevel].nodes.len == 3 + let layout = payload.renders[0.ZLevel].nodes[2].textLayout + doAssert layout.isGlyphView() + doAssert layout.glyphCount() == 2 + doAssert layout.shared[].sourceRunes.bytes == "a🙂b" + doAssert payload.tree.children.len == 1 + doAssert payload.tree.children[0].children[0].children.len == 1 + +suite "FigDraw render ownership": + test "aliased fragments, glyph views and render trees can move between threads": + for iteration in 0 ..< 16: + let channel = newRChan[RenderTransfer](1) + # Preload the channel so the finite worker never waits for readiness. + channel.send(unsafeIsolate(makeTransfer())) + var worker: Thread[RChan[RenderTransfer]] + createThread(worker, releaseOnWorker, channel) + joinThread(worker) diff --git a/tests/tsiwin_dedicated_render.nim b/tests/tsiwin_dedicated_render.nim index 265e645..c950f55 100644 --- a/tests/tsiwin_dedicated_render.nim +++ b/tests/tsiwin_dedicated_render.nim @@ -8,10 +8,37 @@ import figdraw/windowing/siwinshim type FailingContext = ref object of BackendContext kindValue: RendererBackendKind + finishedFrames: bool + +var destroyedContexts: int + +type DestructionProbe = object + armed: bool + +proc `=destroy`(probe: DestructionProbe) = + if probe.armed: + inc destroyedContexts + +type CountedContext = ref object of BackendContext + probe: DestructionProbe + +proc installRendererCallback(window: Window) = + let context = CountedContext(probe: DestructionProbe(armed: true)) + let renderer = newFigRenderer(context, SiwinRenderBackend(window: window)) + window.eventsHandler.onRender = proc(event: RenderEvent) = + doAssert renderer.backendState.window == event.window + +proc exerciseRendererCallback() = + let window = Window() + window.installRendererCallback() + window.eventsHandler.onRender(RenderEvent(window: window)) method kind(ctx: FailingContext): RendererBackendKind = ctx.kindValue +method finishPendingFrames(ctx: FailingContext) = + ctx.finishedFrames = true + method beginFrame( ctx: FailingContext, frameSize: Vec2, clearMain: bool, clearMainColor: Color ) = @@ -22,6 +49,17 @@ method beginFrame( raise newException(ValueError, "preferred backend failed") suite "siwin dedicated rendering": + test "finishing submitted frames delegates to the active backend": + let context = FailingContext() + let renderer = newFigRenderer(context, SiwinRenderBackend()) + renderer.finishPendingFrames() + check context.finishedFrames + + test "releasing a window releases the renderer captured by its callback": + destroyedContexts = 0 + exerciseRendererCallback() + check destroyedContexts == 1 + test "reports platform backend support": check not backendSupportsDedicatedRenderThread(rbOpenGL) when UseMetalBackend and defined(macosx):