Repository navigation
GPU: free effect working textures after each call instead of at frame end - #1523
MatejKozlovsky wants to merge 6 commits into
Conversation
|
Thanks @MatejKozlovsky, the 23 GB → 4.6 GB drop at 4K is a big win. One concern before it lands: each idle pool owner can keep up to |
|
Good catch, you're right, idle owners shouldn't sit outside the accounting. I'll make give_back trim against one global budget and count the idle bytes in the shared total, with a test for the 16-thread case. On order: this one first, then #1441. Combining them actually turned up a real bug (a handed-off texture can go back to its owner while the frame is still being encoded), so #1441 will carry that fix plus a test. Will push both soon. |
…ender Review of storytold#1523: each idle lent owner kept up to POOL_BUDGET of textures of its own, outside the shared accounting, and there were as many idle owners as effect calls ever ran at once (16 frame threads: ~12 GB after a 4K render). Giving a lent owner back now moves its kept textures to the shared free list, so one POOL_BUDGET bounds what the free list and every finished effect call keep. They skip Pool::release's wait for open encoders: only the lent Enc recorded work on them, and that work is submitted or discarded. Each lend is a fresh owner id, so a lease that outlives its call (an image handed to another encoder) is released through Pool::release; this does not rely on the result being read back. Accelerator::render_end (called when an export ends) drops the kept textures.
Review of storytold#1523 (follow-up to 0a884fb): - Pool::make_free rejected a texture when the free list was full and never evicted, so once sizes that no longer occur (per-frame blur bounds, padded layer sizes) filled the budget, every later free was a drop and every take an allocation. It now evicts the least recently freed textures until the new one fits. - A lease read by another Enc (dispatch, copy, readback, display) or handed to a cache is marked escaped and always released through the guarded Pool::release, so a lent call can no longer reuse a texture that a still open encoder reads. - Docs: POOL_BUDGET caps the free list only; textures waiting for open encoders and those of live Encs are outside it. - export_async calls render_end from a drop guard, so the pool is also trimmed when an export panics or its future is dropped. - Tests use a private context with its own pool budget instead of a global override; new tests for stale sizes and an image read by an open encoder.
|
Thanks for the review. I reworked the pool along those lines; the final state is f029766.
Numbers: 4K, old = 13a49c9, new = f029766 (measured as a2a41e76; the code is the same, only comments and commit messages changed since). The measured builds are these commits plus a diagnostics-only patch (memory sampler, hit/miss counters). Each new figure is the median of 3 rounds; old on the 3090 had 2 rounds, so I give its range. Output md5 is identical between old and new in every run. RTX 3090 24 GB / Ryzen 7 5800X:
GTX 1070 8 GB / i7-7700HQ, driver 582.78:
To be upfront: c_effects still peaks above the old code, by 256–512 MB on the 3090 and 256–432 MB on the 1070. d_blurglow is at or below the old peak. The speedup only shows on the 24 GB card. On the 8 GB card the change is neutral. |
… end A GPU frame keeps its encoder open while the CPU renderer runs layers with GPU effects (Gpu::effects -> run_chain). Each call's Enc handed its textures back to the shared pool, where they waited for every open encoder, so all effect intermediates of a frame stayed allocated until the frame ended (17.7 GB by layer 17 of a heavy 4K comp, then OOM). Effect calls now borrow a pool owner (Enc::lent) that is given back once the call's work is submitted or discarded; its kept textures are trimmed to the pool budget, so the rest are freed after each call.
…ender Review of storytold#1523: each idle lent owner kept up to POOL_BUDGET of textures of its own, outside the shared accounting, and there were as many idle owners as effect calls ever ran at once (16 frame threads: ~12 GB after a 4K render). Giving a lent owner back now moves its kept textures to the shared free list, so one POOL_BUDGET bounds what the free list and every finished effect call keep. They skip Pool::release's wait for open encoders: only the lent Enc recorded work on them, and that work is submitted or discarded. Each lend is a fresh owner id, so a lease that outlives its call (an image handed to another encoder) is released through Pool::release; this does not rely on the result being read back. Accelerator::render_end (called when an export ends) drops the kept textures.
Review of storytold#1523 (follow-up to 0a884fb): - Pool::make_free rejected a texture when the free list was full and never evicted, so once sizes that no longer occur (per-frame blur bounds, padded layer sizes) filled the budget, every later free was a drop and every take an allocation. It now evicts the least recently freed textures until the new one fits. - A lease read by another Enc (dispatch, copy, readback, display) or handed to a cache is marked escaped and always released through the guarded Pool::release, so a lent call can no longer reuse a texture that a still open encoder reads. - Docs: POOL_BUDGET caps the free list only; textures waiting for open encoders and those of live Encs are outside it. - export_async calls render_end from a drop guard, so the pool is also trimmed when an export panics or its future is dropped. - Tests use a private context with its own pool budget instead of a global override; new tests for stale sizes and an image read by an open encoder.
…U frees them Evicting the least recently freed texture for every newcomer thrashed when a frame frees more than the budget in a repeating order (blur/glow intermediates): each free evicted the size the next call takes first, so misses came back to the old level and evict + reallocate raised the peak. The free list now evicts only sizes nobody asked for in the last 2 output frames (Accelerator::frame_end, called by the export per frame); otherwise the incoming texture is dropped and the kept set keeps hitting. Evicted bytes count against the budget until the work submitted so far is done, so evictions run at most one budget ahead of the GPU.
On 4K RTX 3090 runs 256 MB matches the old code's peak reserved on d_blurglow (9204 MB) and is 256-512 MB above it on c_effects (6592 vs 6080 MB), while staying faster.
…ugh Enc::record Only the export pipeline called frame_end, so off export (preview, remote frames) the pool's clock stood still, no size went stale and a full free list dropped every newcomer. passes::comp_frame (exports), remote frames and the preview workers now end each frame; test without pool_frame_end. Docs: the frame clock is coarse with many frames in flight; dying only throttles eviction, approximately; no cache path escapes images. Enc::record is the single way to get the encoder outside context.rs and marks the images it names (Enc::uses).
f029766 to
73312c2
Compare
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Reviewed current head 73312c2 against main 8f02f52, including the shared free-list accounting, escaped-lease/open-encoder lifetime, frame-end call sites and export drop guard. I found no blocking correctness issue in the current patch.
Validation on this exact head: the six new pool regressions and four existing GPU output/reuse tests each passed individually (10 passed, 0 failed or ignored). All runs used NVIDIA GeForce RTX 5090 / Vulkan. For the six private-context tests, debugger backtraces confirmed execution reached the private budget setter after GPU initialization, rather than silently returning early. Changed-file formatting, scoped GPU/render/export library Clippy with warnings denied, and source-integrity checks passed.
The performance scope matters: 256 MiB caps the shared free list, not total live or driver-reserved GPU memory. I did not run the 4K performance benchmarks. The later author measurements in comment 6223542652 supersede the opening 23 GB-to-4.6 GB headline: they show mixed peak-memory results and a blur/glow speedup on the 24 GB card, not a universal memory reduction. Please keep that distinction clear in the PR summary/release notes.
This review is for this patch alone, without integrating the unmerged #1441/#1550/#1706 work.
|
Thanks @FredApps for the thorough review and for testing on the RTX 5090. I've updated the PR summary to make the distinction clear: the 256 MiB budget caps only the shared free list of unused textures, not total live or driver-reserved GPU memory. The early 23 GB → 4.6 GB figure is now labelled as superseded by the mixed results in comment 6223542652. |
…er reuses it An effect chain's result handed to the upload cache (adopt_upload) is read by the frame's encoder, which may not be submitted yet. When the cache entry went (its layer buffer died mid-frame, e.g. a 4K export whose layer cache can't keep the buffers), the texture went straight back to the owner Enc's private list, skipping the open-encoder guard, and with a lent owner (storytold#1523) the next same-size chain overwrote it before the frame read it. The lease now remembers it escaped and goes through Pool::release. Tests: a pool-level check that a handed-off texture is not reacquired while another encoder is open, and a frame of five same-size blur + glow layers whose buffers die mid-frame (asserting results really were released mid-frame), vs the CPU and vs the hand-off off. On storytold#1441 alone only the pool test fails without the fix; the frame test covers the interaction with storytold#1523's lent owners. EFFECTCRAFT_NO_HANDOFF is now an AtomicBool so tests can switch it (restored by a guard, also on panic).
|
@bflatastic FredApps approved this on 17 Oct, your idle-owner point is handled by the 16 Oct rework (one shared 256 MB free list), and it still merges cleanly with main. Could it go in, or into the next community batch? The order is still this first, then #1441. #1441 is based on main, not on this branch, but its 4K gain only shows on top of this one. Once this lands, I'll push the one-line |
This branch has not been deployed
3 participants
Summary
Effect working textures are now freed after each
Gpu::effectscall instead of being kept until the frame ends, and idle pool owners no longer keep their own sets: unused textures go to one shared free list capped at 256 MiB (POOL_BUDGET).Scope of the budget: 256 MiB caps the shared free list of unused textures only. It does not cap total live GPU memory or driver/allocator-reserved memory; textures held by live
Encs and textures waiting on open encoders are outside it.Current results (comment 6223542652, 4K, old = 13a49c9 (this PR's first commit) vs new = f029766, the same patches as the current head 73312c2) are mixed on peak memory, not a universal memory reduction:
Problem
On heavy 4K comps the GPU runs out of memory on frame 0 (RTX 3090, 24 GB), and every frame then renders on the CPU. A per-layer probe of the texture pool on
c_effectsshows where the memory goes:releasedlist: grows by about 16 textures (about 1.1 GB) per layer, reaching 252 textures (17.7 GB) by layer 17.Those textures come from the effects path,
Gpu::effects→run_chain. The CPU renderer calls it for each layer with GPU effects, and each call uses a freshEnc. When thatEncdrops, its textures go back to the shared pool. There they wait for every encoder that was open at the time of release to be submitted. The frame's own encoder stays open for the whole frame, so every effect intermediate stays allocated until the frame ends.Change
First commit (13a49c9):
Gpu::effectsnow runs on anEncthat borrows a pool owner, oneEncat a time, and gives it back once the call's work is submitted or discarded (Enc::lent). Giving it back trims the owner's kept textures to the existing 768 MB pool budget, so a 4K effect chain's intermediates are freed after each call instead of piling up until the frame ends. This is safe because the call's work is already queued before the textures are handed on, and its images never reach another encoder: the result is read back into a CPU buffer. 38 lines incrates/gpu/src/context.rsand 1 line incrates/gpu/src/lib.rs. (These line counts and the 768 MB per-owner budget describe the first commit only.)Later commits (details in comment 6223542652): one shared free list with a 256 MiB budget; eviction only of sizes not requested in the last
STALE_FRAMES = 2output frames (otherwise the incoming texture is dropped); a dying-counter throttle so evicted bytes count against the budget until submitted work completes;frame_endon the multi-frame render paths (exports, remote frames, preview workers); aRenderEnddrop guard inexport_asyncthat empties the free list after an export; andEnc::recordas the single encoder entry point, marking escaped leases so they go through the guarded release (covers the #1441 open-encoder case).Early results (first commit 13a49c9 vs main, superseded)
These were measured before the pool rework and are kept for history. The early peak figure (more than 23 GB / OOM on main → 4.6 GB) is superseded by the current results in the Summary.
Vulkan, RTX 3090, 4K, 6 frames, 16 threads, H.264, warm shader cache.
c_effectsdropped from more than 23 GB (OOM on main) to 4.6 GB, and the GPU rendered every frame.Notes
Testing
First commit:
cargo fmt --checkandclippy --no-deps -D warningsare clean.cargo test -p effectcraft-gpugives 210 passed and 2 failed (tests_fx_pixel2::stylize,tests_fx_stylize::posterize_thresholds_strobe_vignette). Both also fail on unmodified main on the same software adapter.After the rebase onto main 8f02f52 (head 73312c2, no code changes):
cargo test -p effectcraft-gpugives 218 passed and the same 2 failed on the software adapter; the 6 pool tests pass; fmt and clippy are clean.