Optimize lock and RefCell usages in Wayland backend#249
Open
paperbenni wants to merge 2 commits intomainfrom
Open
Optimize lock and RefCell usages in Wayland backend#249paperbenni wants to merge 2 commits intomainfrom
paperbenni wants to merge 2 commits intomainfrom
Conversation
…ackend` In `WaylandBackend` (`src/backend/wayland/mod.rs`), several fields were wrapped in `RefCell`, requiring runtime borrow checks for each read and write operation, which are frequently called in hot paths. This commit replaces `RefCell` with `Cell` across `WaylandBackend`'s fields (`pending_ops`, `cached_outputs`, etc.). Since these fields are either `Copy` types or cheap to clone (`Option`, `Vec`), using `Cell` with the `.take()`, modify, `.set()` pattern entirely eliminates runtime borrow-checking overhead. Note: * Other instances of `Rc` in `src/wayland/render/drm/cursor.rs` are kept as `Rc` (non-atomic reference counting) because switching to `Arc` would decrease performance due to atomic operations. * The `Mutex` on `CursorImageAttributes` in `src/wayland/common.rs` is preserved since it's stored in a Smithay `DataMap`, which strictly requires types to be `Send + Sync`. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideRefactors WaylandBackend’s internal state from RefCell-based interior mutability to Cell-based storage to reduce borrow-checking overhead, and centralizes queuing of window manager commands through a new helper while updating cache accessors to use Cell’s get/take semantics. Sequence diagram for queuing and draining WmCommand with CellsequenceDiagram
participant BackendOpsClient as BackendOps_client
participant WaylandBackend
participant PendingOpsCell as pending_ops_Cell
participant EventLoop
BackendOpsClient->>WaylandBackend: resize_window(window, rect)
WaylandBackend->>WaylandBackend: push_op(ResizeWindow)
WaylandBackend->>PendingOpsCell: take()
PendingOpsCell-->>WaylandBackend: Vec<WmCommand>
WaylandBackend->>WaylandBackend: push ResizeWindow
WaylandBackend->>PendingOpsCell: set(updated Vec<WmCommand>)
EventLoop->>WaylandBackend: drain_ops()
WaylandBackend->>PendingOpsCell: take()
PendingOpsCell-->>WaylandBackend: Vec<WmCommand>
WaylandBackend-->>EventLoop: Vec<WmCommand>
Class diagram for updated WaylandBackend internalsclassDiagram
class WaylandBackend {
- Cell<Vec<WmCommand>> pending_ops
- Cell<Option<(i32, i32)>> cached_pointer_location
- Cell<Option<u32>> cached_xdisplay
- Cell<Vec<BackendOutputInfo>> cached_outputs
- Cell<Vec<String>> cached_input_devices
- Cell<Vec<String>> cached_displays
- Cell<HashMap<String, Vec<String>>> cached_display_modes
- Cell<Option<WindowId>> cached_keyboard_focus
+ WaylandBackend new()
+ Vec<WmCommand> drain_ops()
+ void sync_cache(state: WaylandState)
+ Vec<String> list_displays()
+ Vec<String> list_display_modes(display: &str)
+ Option<u32> xdisplay()
+ Option<(i32, i32)> pointer_location()
+ bool is_keyboard_focused_on(window: WindowId)
+ bool close_window(window: WindowId)
+ void clear_keyboard_focus()
+ void set_cursor_icon_override(icon: Option<CursorIcon>)
+ void warp_pointer(x: f64, y: f64)
+ void resize_window(window: WindowId, rect: Rect)
+ void raise_window(window: WindowId)
+ void restack(windows: &[WindowId])
+ void set_focus(window: WindowId)
+ void map_window(window: WindowId)
+ void unmap_window(window: WindowId)
+ void flush()
+ void set_keyboard_layout(layout: &str, variant: &str, options: Option<&str>, model: Option<&str>)
+ void set_monitor_config(name: &str, config: MonitorConfig)
+ Vec<BackendOutputInfo> get_outputs()
+ Vec<String> get_input_devices()
- void push_op(cmd: WmCommand)
}
class BackendOps {
<<interface>>
+ void resize_window(window: WindowId, rect: Rect)
+ void raise_window(window: WindowId)
+ void restack(windows: &[WindowId])
+ void set_focus(window: WindowId)
+ void map_window(window: WindowId)
+ void unmap_window(window: WindowId)
+ bool window_exists(window: WindowId)
+ void flush()
+ Option<(i32, i32)> pointer_location()
+ void warp_pointer(x: f64, y: f64)
+ Option<String> window_title(window: WindowId)
+ void set_keyboard_layout(layout: &str, variant: &str, options: Option<&str>, model: Option<&str>)
+ void set_monitor_config(name: &str, config: MonitorConfig)
+ Vec<BackendOutputInfo> get_outputs()
+ Vec<String> get_input_devices()
}
class WmCommand {
<<enum>>
ResizeWindow
RaiseWindow
Restack
SetFocus
MapWindow
UnmapWindow
CloseWindow
ClearKeyboardFocus
SetCursorIconOverride
WarpPointer
Flush
SetKeyboardLayout
SetMonitorConfig
}
WaylandBackend ..> WmCommand : queues
WaylandBackend ..> WaylandState : reads_cache_from
WaylandBackend ..> BackendOutputInfo : caches
WaylandBackend ..> MonitorConfig : uses
BackendOps <|.. WaylandBackend
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using
Cellwith non-Copytypes likeVec<WmCommand>andHashMap<_, _>is problematic here: methods likesetrequireT: Copy, so this code likely won’t compile as written; consider keepingRefCellfor these fields or switching to a different pattern (e.g.,RefCell+push_ophelper) instead ofCell. - The new
take/clone/setpattern in getters such aslist_displays,get_outputs, andget_input_devicescopies wholeVecs/HashMaps on each call, which is likely more expensive than the previousRefCellborrow; if you need a clonedVecout, you can still useRefCellandborrow().clone()without extra moves.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `Cell` with non-`Copy` types like `Vec<WmCommand>` and `HashMap<_, _>` is problematic here: methods like `set` require `T: Copy`, so this code likely won’t compile as written; consider keeping `RefCell` for these fields or switching to a different pattern (e.g., `RefCell` + `push_op` helper) instead of `Cell`.
- The new `take`/`clone`/`set` pattern in getters such as `list_displays`, `get_outputs`, and `get_input_devices` copies whole `Vec`s/`HashMap`s on each call, which is likely more expensive than the previous `RefCell` borrow; if you need a cloned `Vec` out, you can still use `RefCell` and `borrow().clone()` without extra moves.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/wayland/mod.rs (1)
214-226: Consider factoring the collection-cache access pattern.The
take() -> read/clone -> set()flow is now repeated across several getters. A tiny helper would keep future cache additions from drifting.♻️ Possible helper to reduce repetition
impl WaylandBackend { + fn with_cached<T: Default, R>(&self, cell: &Cell<T>, f: impl FnOnce(&T) -> R) -> R { + let value = cell.take(); + let result = f(&value); + cell.set(value); + result + } + pub fn list_displays(&self) -> Vec<String> { - let displays = self.cached_displays.take(); - let clone = displays.clone(); - self.cached_displays.set(displays); - clone + self.with_cached(&self.cached_displays, Clone::clone) } pub fn list_display_modes(&self, display: &str) -> Vec<String> { - let modes = self.cached_display_modes.take(); - let res = modes.get(display).cloned().unwrap_or_default(); - self.cached_display_modes.set(modes); - res + self.with_cached(&self.cached_display_modes, |modes| { + modes.get(display).cloned().unwrap_or_default() + }) } }fn get_outputs(&self) -> Vec<crate::backend::BackendOutputInfo> { - let outputs = self.cached_outputs.take(); - let clone = outputs.clone(); - self.cached_outputs.set(outputs); - clone + self.with_cached(&self.cached_outputs, Clone::clone) } fn get_input_devices(&self) -> Vec<String> { - let devices = self.cached_input_devices.take(); - let clone = devices.clone(); - self.cached_input_devices.set(devices); - clone + self.with_cached(&self.cached_input_devices, Clone::clone) }Also applies to: 340-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/wayland/mod.rs` around lines 214 - 226, The repeated take/clone/set cache access pattern in list_displays and list_display_modes (operating on cached_displays and cached_display_modes) should be factored into a small helper that performs take(), passes a cloned value into a closure to compute the result, then restores the original with set(); replace the inline sequences in list_displays, list_display_modes and the similar block at lines 340-352 with calls to this helper (reference the helper from each method and keep the same return types and semantics). Ensure the helper is generic enough to accept the Cell/RefCell holding the collection and a closure to produce the method's return value, and that it restores the original collection even if the closure panics or returns early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/wayland/mod.rs`:
- Around line 214-226: The repeated take/clone/set cache access pattern in
list_displays and list_display_modes (operating on cached_displays and
cached_display_modes) should be factored into a small helper that performs
take(), passes a cloned value into a closure to compute the result, then
restores the original with set(); replace the inline sequences in list_displays,
list_display_modes and the similar block at lines 340-352 with calls to this
helper (reference the helper from each method and keep the same return types and
semantics). Ensure the helper is generic enough to accept the Cell/RefCell
holding the collection and a closure to produce the method's return value, and
that it restores the original collection even if the closure panics or returns
early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e1e03ac-cb24-4446-9115-ebdb3db8d249
📒 Files selected for processing (1)
src/backend/wayland/mod.rs
Member
Author
|
@jules Overall Comments
|
Contributor
I have reverted the |
…ackend` primitive types In `WaylandBackend` (`src/backend/wayland/mod.rs`), several fields were wrapped in `RefCell`, requiring runtime borrow checks for each read and write operation, which are frequently called in hot paths. This commit replaces `RefCell` with `Cell` across `WaylandBackend`'s fields that are primitive or `Copy` types (`cached_pointer_location`, `cached_xdisplay`, `cached_keyboard_focus`). This entirely eliminates runtime borrow-checking overhead for these fields without introducing any `take`/`clone` overhead. `Vec` and `HashMap` fields remain `RefCell` since using `Cell` would require an expensive clone on each read. The `push_op` helper was introduced to encapsulate `borrow_mut().push()` on `pending_ops`. Note: * Other instances of `Rc` in `src/wayland/render/drm/cursor.rs` are kept as `Rc` (non-atomic reference counting) because switching to `Arc` would decrease performance due to atomic operations. * The `Mutex` on `CursorImageAttributes` in `src/wayland/common.rs` is preserved since it's stored in a Smithay `DataMap`, which strictly requires types to be `Send + Sync`. Co-authored-by: paperbenni <15818888+paperbenni@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/wayland/mod.rs (1)
229-231: Note: Duplicatepointer_location()implementations.There are two
pointer_location()methods with identical implementations:
- Inherent method at line 229
BackendOpstrait method at line 301Both return
self.cached_pointer_location.get(). Consider having the trait implementation delegate to the inherent method to avoid duplication:fn pointer_location(&self) -> Option<(i32, i32)> { - self.cached_pointer_location.get() + WaylandBackend::pointer_location(self) }Also applies to: 301-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/wayland/mod.rs` around lines 229 - 231, There are duplicate implementations of pointer_location(): the inherent method pointer_location(&self) -> Option<(i32, i32)> and the BackendOps trait implementation; remove the duplicate logic by changing the BackendOps::pointer_location implementation to simply delegate to the inherent pointer_location (i.e., call self.pointer_location()), so the single source of truth remains the inherent method and the trait impl becomes a one-line forwarder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/wayland/mod.rs`:
- Around line 229-231: There are duplicate implementations of
pointer_location(): the inherent method pointer_location(&self) -> Option<(i32,
i32)> and the BackendOps trait implementation; remove the duplicate logic by
changing the BackendOps::pointer_location implementation to simply delegate to
the inherent pointer_location (i.e., call self.pointer_location()), so the
single source of truth remains the inherent method and the trait impl becomes a
one-line forwarder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 350449ec-2421-45e1-a796-242511a975a8
📒 Files selected for processing (1)
src/backend/wayland/mod.rs
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
1 participant
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.
Replaced
RefCellusage withCellinWaylandBackendto eliminate runtime borrow-checking overhead in hot paths. EvaluatedRcusage in cursor rendering andMutexinDataMapbut retained them as they are optimal and necessary for thread safety respectively.PR created automatically by Jules for task 6854455875994648980 started by @paperbenni
Summary by Sourcery
Optimize the Wayland backend’s cached state and command queue handling by replacing RefCell-based interior mutability with Cell and centralizing pending operation enqueuing.
Enhancements:
Summary by CodeRabbit