Open
Conversation
The `calloop::EventLoop::run` method was being called with a timeout of `Duration::from_millis(16)` in both the DRM and Winit Wayland backends. This caused the event loop to wake up and iterate roughly 60 times a second even when completely idle, resulting in around 9% continuous CPU usage. Changing the timeout parameter to `None` allows the loop to block efficiently on `epoll_wait` (or similar mechanisms) until actual file descriptor events occur, drastically reducing idle CPU overhead without affecting responsiveness. Unused `std::time::Duration` imports were also removed. 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 guide (collapsed on small PRs)Reviewer's GuideReplaces the fixed 16ms timeout in Wayland event loop runners with a blocking run (no timeout) to eliminate unnecessary wakeups and reduce idle CPU usage in the Wayland DRM and winit backends. Sequence diagram for Wayland event loop with blocking runsequenceDiagram
participant WaylandBackend
participant CalloopEventLoop as CalloopEventLoop_run
participant Kernel as OS_Scheduler
WaylandBackend->>CalloopEventLoop: run(None, state, callback)
activate CalloopEventLoop
CalloopEventLoop-->>OS_Scheduler: block_until_events()
OS_Scheduler-->>CalloopEventLoop: wake_on_event
CalloopEventLoop->>WaylandBackend: invoke callback(state)
WaylandBackend->>WaylandBackend: process_completed_crtcs
WaylandBackend->>WaylandBackend: process_pending_libinput_events
WaylandBackend-->>CalloopEventLoop: return
loop While not terminated
CalloopEventLoop-->>OS_Scheduler: block_until_events()
OS_Scheduler-->>CalloopEventLoop: wake_on_event
CalloopEventLoop->>WaylandBackend: invoke callback(state)
end
deactivate CalloopEventLoop
Flow diagram for DRM and winit Wayland event loops using blocking runflowchart TD
A[Start Wayland backend] --> B[Initialize state]
B --> C[Create Calloop event_loop]
C --> D["Call event_loop.run(None, state, callback)"]
D --> E[Block until events are available]
E --> F{Events pending?}
F -->|Yes| G[Invoke callback with state]
G --> H[DRM backend: process_completed_crtcs]
H --> I[DRM backend: process_pending_libinput_events]
I --> J[Return to event loop]
F -->|No| E
J --> E
subgraph Winit_backend
G2[Invoke callback with mut state]
G2 --> K[Dispatch winit events]
K --> L[Handle resize and other wm events]
L --> J2[Return to event loop]
J2 --> E
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughBoth Wayland event-loop implementations are modified to remove fixed 16ms tick scheduling. The Changes
Possibly related PRs
Poem
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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:
- Before switching the timeout to
None, double-check whether any logic inprocess_completed_crtcsorprocess_pending_libinput_eventsimplicitly relied on a periodic 16ms tick (e.g., animations, timeouts, or housekeeping) that now may not run unless input/Wayland events arrive. - If you still need occasional work to happen in the absence of external events, consider using calloop timers or idle callbacks instead of a global fixed timeout, so you keep low idle CPU usage while preserving any needed periodic behavior.
- It may be worth confirming that all relevant event sources (Wayland, libinput, winit) are registered with the same calloop event loop so that
.run(None, ...)will reliably wake on user/display activity and not block longer than intended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Before switching the timeout to `None`, double-check whether any logic in `process_completed_crtcs` or `process_pending_libinput_events` implicitly relied on a periodic 16ms tick (e.g., animations, timeouts, or housekeeping) that now may not run unless input/Wayland events arrive.
- If you still need occasional work to happen in the absence of external events, consider using calloop timers or idle callbacks instead of a global fixed timeout, so you keep low idle CPU usage while preserving any needed periodic behavior.
- It may be worth confirming that all relevant event sources (Wayland, libinput, winit) are registered with the same calloop event loop so that `.run(None, ...)` will reliably wake on user/display activity and not block longer than intended.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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wayland/runtime/winit.rs`:
- Around line 88-89: The event loop currently calls event_loop.run(None, &mut
state, move |mut state| { ... }) which blocks indefinitely and prevents
render_frame()/tick_window_animations() from progressing during idle periods;
either restore a 16ms timeout on the run call (replace None with
Some(Duration::from_millis(16))) or register a calloop timer source that wakes
the loop every ~16ms while animations are active so render_frame() can drive
tick_window_animations(); update the logic around event_loop.run and the
animation-active flag in state to start/stop the timer appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99d6ce9d-d202-40be-8ce5-7f0c2261b44b
📒 Files selected for processing (2)
src/wayland/runtime/drm.rssrc/wayland/runtime/winit.rs
Comment on lines
88
to
+89
| event_loop | ||
| .run(Duration::from_millis(16), &mut state, move |mut state| { | ||
| .run(None, &mut state, move |mut state| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if winit backend has any timer or frame-request mechanism for continuous rendering
echo "=== Searching for animation/redraw wake mechanisms in winit backend ==="
rg -n -C3 'RedrawRequested|request_redraw|timer|Timer|animate' src/wayland/runtime/winit.rs src/wayland/render/winit.rs
echo ""
echo "=== Check if winit backend uses calloop timers ==="
rg -n 'calloop.*timer|Timer::' src/wayland/runtime/
echo ""
echo "=== Check animation state handling ==="
rg -n -C2 'has_active.*animation|tick.*animation' src/wayland/runtime/Repository: instantOS/instantWM
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Examine the winit event_loop.run() callback implementation
echo "=== Winit event loop callback implementation ==="
sed -n '88,200p' src/wayland/runtime/winit.rs | cat -n
echo ""
echo "=== Check if render_frame is called in winit backend ==="
rg -n 'render_frame' src/wayland/runtime/winit.rs src/wayland/render/
echo ""
echo "=== Check animation handling in common.rs ==="
sed -n '1,50p' src/wayland/runtime/common.rs | cat -n
echo ""
echo "=== Check if winit backend needs to handle animations differently ==="
rg -n -C5 'has_active_window_animations' src/wayland/runtime/common.rs src/wayland/runtime/winit.rsRepository: instantOS/instantWM
Length of output: 8207
🏁 Script executed:
#!/bin/bash
# Search for where tick_window_animations is called
echo "=== All calls to tick_window_animations ==="
rg -n 'tick_window_animations' --type rust
echo ""
echo "=== Check if winit render_frame handles animations ==="
sed -n '26,100p' src/wayland/render/winit.rs | cat -n
echo ""
echo "=== Check WaylandState for animation methods ==="
rg -n 'fn.*animation|tick_window_animations|has_active_window_animations' src/backend/wayland/Repository: instantOS/instantWM
Length of output: 4319
🏁 Script executed:
#!/bin/bash
# Check if winit event loop integrates with wayland client sockets as event sources
echo "=== Search for calloop socket/source registration in winit backend ==="
rg -n 'insert|register|add_source|with_source' src/wayland/runtime/winit.rs
echo ""
echo "=== Check for any timer-like sources in winit ==="
rg -n 'Timer|timer|source|Source' src/wayland/runtime/winit.rs | head -20
echo ""
echo "=== Examine full winit event loop setup (first 100 lines) ==="
head -100 src/wayland/runtime/winit.rs | cat -nRepository: instantOS/instantWM
Length of output: 5086
Restore the 16ms timeout or add a timer source to maintain animation frame timing.
The winit backend's render_frame() calls tick_window_animations(), but without the timeout, the loop blocks on epoll_wait until an external event arrives. Unlike the DRM backend (which has VBlank events), winit has no frame timing source. Animations will stall during idle periods—e.g., window fade-in animations with no user input will freeze mid-animation.
Either restore the 16ms timeout or register a calloop timer source to wake the loop periodically during active animations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wayland/runtime/winit.rs` around lines 88 - 89, The event loop currently
calls event_loop.run(None, &mut state, move |mut state| { ... }) which blocks
indefinitely and prevents render_frame()/tick_window_animations() from
progressing during idle periods; either restore a 16ms timeout on the run call
(replace None with Some(Duration::from_millis(16))) or register a calloop timer
source that wakes the loop every ~16ms while animations are active so
render_frame() can drive tick_window_animations(); update the logic around
event_loop.run and the animation-active flag in state to start/stop the timer
appropriately.
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.
Resolves an issue where instantWM was consuming ~9% idle CPU on the Wayland backend due to a 16ms timeout in
calloop::EventLoop::run(). Removing the timeout allows the event loop to block correctly.PR created automatically by Jules for task 3785322044631998059 started by @paperbenni
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit