Repository navigation
improve: reuse GPU layer pipelines across export preview frames; coalesce export progress updates - #2482
Conversation
…esce export progress updates Preview frames rebuilt RendererLayers (full shader/pipeline compilation, ~0.25-0.5s) on every scrub. Preview now takes pooled layers via the same LAYERS_POOL mechanism the export path uses, keyed per project, returning them after a clean render. The three export progress drain loops issued a full model update + cx.notify() per progress message (~60/s) while only the latest value survives each 50ms wake. They now drain to the latest message and update once per wake: 3x fewer render-tree rebuilds, identical visible values. Benchmarks: crates/export/examples/preview-layers-benchmark.rs, apps/desktop-gpui/examples/export-progress-coalesce-benchmark.rs
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let mut layers = layers_reuse_key | ||
| .as_deref() | ||
| .and_then(|key| cap_rendering::take_reusable_layers(&render_constants.device, key)) |
There was a problem hiding this comment.
After previewing a clip with a cursor, scrubbing to a clip without cursor events can draw the old cursor. The pooled layers keep CursorLayer.bind_group, but CursorLayer.prepare returns without clearing it when interpolated_cursor is None. Previously, each preview started with fresh layers. Clear the cursor’s draw state when taking layers or before returning from that branch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/export/src/preview.rs
Line: 292-294
Comment:
**Old cursor survives preview changes**
After previewing a clip with a cursor, scrubbing to a clip without cursor events can draw the old cursor. The pooled layers keep `CursorLayer.bind_group`, but `CursorLayer.prepare` returns without clearing it when `interpolated_cursor` is `None`. Previously, each preview started with fresh layers. Clear the cursor’s draw state when taking layers or before returning from that branch.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| println!("old updates: {old_updates}"); | ||
| println!("new updates: {new_updates}"); | ||
| println!( | ||
| "update reduction: {:.1}x (each avoided update skips a full render-tree rebuild)", |
There was a problem hiding this comment.
The benchmark reports fewer full editor renders, but it only counts mock_update calls. It never creates a GPUI view or calls cx.notify(), so it cannot measure that saving. Maintainers could mistake fewer field writes for fewer editor renders. Report fewer model updates instead, or measure actual renders in GPUI.
| "update reduction: {:.1}x (each avoided update skips a full render-tree rebuild)", | |
| "model update reduction: {:.1}x (editor renders are not measured)", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/examples/export-progress-coalesce-benchmark.rs
Line: 122
Comment:
**Benchmark overstates render savings**
The benchmark reports fewer full editor renders, but it only counts `mock_update` calls. It never creates a GPUI view or calls `cx.notify()`, so it cannot measure that saving. Maintainers could mistake fewer field writes for fewer editor renders. Report fewer model updates instead, or measure actual renders in GPUI.
```suggestion
"model update reduction: {:.1}x (editor renders are not measured)",
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /// Take a set of GPU layer pipelines previously returned to the pool for | ||
| /// `key` on this device, with per-frame state reset. Returns `None` when the | ||
| /// pool holds nothing reusable, in which case the caller should build a fresh | ||
| /// [`RendererLayers`] and [`return_reusable_layers`] it afterwards. | ||
| /// |
There was a problem hiding this comment.
The opening documentation for take_reusable_layers narrates the function and its return type. AGENTS.md explicitly bans comments that narrate code or paraphrase parameters. Remove this routine description and keep only the non-obvious reuse and failure-state requirements. This repository requirement must be satisfied before merging.
| /// Take a set of GPU layer pipelines previously returned to the pool for | |
| /// `key` on this device, with per-frame state reset. Returns `None` when the | |
| /// pool holds nothing reusable, in which case the caller should build a fresh | |
| /// [`RendererLayers`] and [`return_reusable_layers`] it afterwards. | |
| /// |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/rendering/src/lib.rs
Line: 1158-1162
Comment:
**New comments repeat the code**
The opening documentation for `take_reusable_layers` narrates the function and its return type. `AGENTS.md` explicitly bans comments that narrate code or paraphrase parameters. Remove this routine description and keep only the non-obvious reuse and failure-state requirements. This repository requirement must be satisfied before merging.
```suggestion
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…onest benchmark Greptile review follow-ups: - CursorLayer::prepare returned early without clearing bind_group when interpolated_cursor was None or the cursor texture failed to load. Harmless with fresh layers per frame, but pooled layers would draw a stale cursor after scrubbing to a cursorless clip. Now cleared on both paths. - Trimmed take_reusable_layers docs to the non-obvious pool-key invariant per AGENTS.md. - Benchmark now states exactly what it measures (update invocations) vs what it does not (cx.notify render cost).
|
Please get the current head to 5/5 with Greptile before we merge. I can see the cursor-state reset fix in 89a68d6; please have Greptile re-review that change, verify cursor-present → cursor-absent preview transitions, and correct the benchmark claims so model-update counts are not presented as measured editor renders. |
|
Addressed all three Greptile items in 89a68d6: Cursor state (P1): verified the full transition. Benchmark claims (P2): the example now states exactly what it measures (update invocations, exact at 3.0x) vs what it does not ( Doc comments (P2): trimmed to the non-obvious pool-key invariant per AGENTS.md. @greptile please re-review the current head. |
| //! Baseline mirrors what `render_preview_frame` did before pooling: | ||
| //! `RendererLayers::new_with_options` on every frame, paying full | ||
| //! shader/pipeline compilation each time. Improved measures the | ||
| //! `take_reusable_layers` / `return_reusable_layers` round trip used now. |
There was a problem hiding this comment.
This header describes the old and new implementation, as does the header in apps/desktop-gpui/examples/export-progress-coalesce-benchmark.rs. AGENTS.md explicitly bans comments that narrate code or describe the change. Remove those summaries; keep useful measurement limits and setup requirements. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/export/examples/preview-layers-benchmark.rs
Line: 3-6
Comment:
**Benchmark comments repeat the code**
This header describes the old and new implementation, as does the header in `apps/desktop-gpui/examples/export-progress-coalesce-benchmark.rs`. `AGENTS.md` explicitly bans comments that narrate code or describe the change. Remove those summaries; keep useful measurement limits and setup requirements. This repository requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Trimmed both benchmark headers to measurement-only (what it measures + how to run it), no old/new narrative, per AGENTS.md. @greptile please re-review. |
|
@Wraient is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks @Wraient, the cursor reset and the coalesced progress loops look right. One thing before we can take it: |
…e pool Richie's review: render_preview_frame handed layers to the process-wide LAYERS_POOL keyed by preview:<path>, and nothing removed the entry when the editor closed, leaking a full RendererLayers after the window was gone. Keep the reusable layers on the EditorInstance as Mutex<Option<RendererLayers>>, taken and put back around the preview render, so they are freed with the editor. The config path (fresh device per call) is unaffected.
…s-pool-and-export-progress-coalesce
|
@greptile please re-review |
…s-pool-and-export-progress-coalesce
|
@greptile please re-review |
| if let Some(cache) = layers_cache { | ||
| if let Ok(mut guard) = cache.lock() { | ||
| *guard = Some(layers); | ||
| } | ||
| } |
There was a problem hiding this comment.
The cache write-back nests two if let checks. AGENTS.md explicitly requires combining collapsible checks. Use if let Some(cache) = layers_cache && let Ok(mut guard) = cache.lock() instead. This repository requirement must be satisfied before merging.
| if let Some(cache) = layers_cache { | |
| if let Ok(mut guard) = cache.lock() { | |
| *guard = Some(layers); | |
| } | |
| } | |
| if let Some(cache) = layers_cache | |
| && let Ok(mut guard) = cache.lock() | |
| { | |
| *guard = Some(layers); | |
| } |
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/export/src/preview.rs
Line: 410-414
Comment:
**Nested checks break Rust rules**
The cache write-back nests two `if let` checks. `AGENTS.md` explicitly requires combining collapsible checks. Use `if let Some(cache) = layers_cache && let Ok(mut guard) = cache.lock()` instead. This repository requirement must be satisfied before merging.
```suggestion
if let Some(cache) = layers_cache
&& let Ok(mut guard) = cache.lock()
{
*guard = Some(layers);
}
```
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…e-back Addresses Greptile P2 on the current head: AGENTS.md requires combining collapsible checks. No behavior change.
|
@greptile please re-review |
…s-pool-and-export-progress-coalesce
|
@greptile please re-review |
…s-pool-and-export-progress-coalesce
|
@greptile please re-review |
|
Merged current main in (e1b5ff4) and fmt is clean. The CI runs on this head are queued behind first-time-contributor approval (@richiemcilroy). Once approved, Greptile is already re-requested on the new head. |
|
Both asks addressed on the current head:
Greptile is green on this head and CI passes. Ready for another look whenever you are. |
|
Correction to my earlier note: the CI workflows (CI, Perf budget, A/V Sync Tests, Verify recording, Engine verify) on this head are still sitting at "action required" behind the first-time-contributor approval gate, so they have not actually run yet, I should not have said CI passes. @richiemcilroy, the runs need your approval before they go. Greptile's review check did pass on this head (16:12 UTC) with no new findings, and its open P2 (nested if-let) is addressed in 81cf678 on this head. |
|
Thanks @Wraient, moving the preview layers onto |
take_reusable_layers and return_reusable_layers were only used by the preview-layers benchmark, which measured the process-wide pool the editor preview no longer uses. Drop both examples as well.
|
Done in 159ebdc: removed Also updated the PR body: the Benchmarks section and the old pool description are gone, and section 1 now describes the editor-owned layers as they stand. @greptile please re-review |
|
@greptile please re-review |
…s-pool-and-export-progress-coalesce
…s-pool-and-export-progress-coalesce
|
@greptile please re-review |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Summary
Two independent, zero-behavior-change performance wins in the GPUI app's export surface:
crates/export/src/preview.rs,crates/rendering/src/lib.rsapps/desktop-gpui/src/editor_export.rs(3 drain loops)1. Editor-owned preview layers
render_preview_framebuilt a freshRendererLayerson every call, paying full shader/pipeline compilation each time (~0.25–0.5 s per the note atcrates/rendering/src/lib.rs). The layers now live on the editor instance (EditorInstance, asMutex<Option<RendererLayers>>): taken and put back around each preview render, reused across scrub frames, and freed when the editor closes. The config path still creates fresh layers per call.Also fixed a latent bug this exposed:
CursorLayer::preparereturned early without clearingbind_groupwhen a frame had no cursor, which would have drawn a stale cursor after pooling. Now cleared on both early-return paths.2. Coalesced progress drain
The file-export, link-export render, and link-export upload loops each did a full model update +
cx.notify()(entire editor render-tree rebuild) for every progress message (~60/s), while only the latest value survives each 50 ms wake. All three now drain to the latest message and update once per wake.Risk
reset_frame_state()on every take; cursorbind_groupcleared when absentTest plan
cargo fmt --all -- --checkclean
The PR appears safe to merge; all previous findings are addressed and no new blocking issues were found.Summary
The PR keeps GPU layers on each editor for reuse across export previews and applies only the latest progress value on each wake.
Reviews (11) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile