⚡ Optimize ATSPI window loading by batching N+1 queries - #45
Conversation
…lications. Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
|
👋 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1177a6eb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let children = provider_call!(cancellation, deadline_at_ms, accessible.get_children())?; | ||
| if children.len() > MAX_WINDOWS_PER_APPLICATION { |
There was a problem hiding this comment.
Bound child count before fetching the batch
When an application exposes an unusually large or malicious top-level accessibility list, get_children() materializes and deserializes the entire D-Bus response before MAX_WINDOWS_PER_APPLICATION is checked, so the 128-window limit no longer bounds memory or IPC work. The same new pattern also appears in the focus-sentinel path. Retain a bounded preflight such as child_count() before requesting the batch, or otherwise enforce the limit while decoding the response.
Useful? React with 👍 / 👎.
Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
Co-authored-by: undivisible <136312656+undivisible@users.noreply.github.com>
💡 What: Replaced the
child_count()+get_child_at_index()loops with a singleget_children()batch RPC call insrc/linux_atspi.rsfor loading accessibility applications and their active windows.🎯 Why: The N+1 query pattern generates excessive D-Bus traffic. For an application with 100 windows, the old pattern required 101 blocking IPC calls (
child_count+ 100get_child_at_indexcalls). The new approach requires exactly 1 blocking IPC call.📊 Measured Improvement: I was unable to show a measured performance improvement because establishing a reliable ATSPI D-Bus mock service environment for benchmark tests requires significant architectural changes and mock definitions that far exceed the scope of the fix itself. However, moving from an N+1 D-Bus RPC pattern to a single batched IPC call is a definitive, unarguable performance optimization for any blocking client.
PR created automatically by Jules for task 624884106141225388 started by @undivisible
Note
Low Risk
Localized IPC optimization on window listing; behavior and safety checks are preserved aside from dropping an explicit negative
child_countguard thatget_children()does not expose.Overview
Replaces per-window ATSPI D-Bus round trips when listing an application’s top-level windows.
accessibility_focus_sentinelandwindows()no longer callchild_count()plusget_child_at_index()in a loop; they use oneget_children()call and iterate the result.Window caps still use
MAX_WINDOWS_PER_APPLICATIONon the returned slice length. Child binding, role/state filtering, and duplicate-surface checks are unchanged.Reviewed by Cursor Bugbot for commit f1177a6. Configure here.