Skip to content

Coalesce idempotent refreshSession calls triggered by event handlers#1249

Closed
rickyz wants to merge 1 commit intonikitabobko:mainfrom
rickyz:coalesce_refresh
Closed

Coalesce idempotent refreshSession calls triggered by event handlers#1249
rickyz wants to merge 1 commit intonikitabobko:mainfrom
rickyz:coalesce_refresh

Conversation

@rickyz
Copy link
Copy Markdown
Collaborator

@rickyz rickyz commented Mar 30, 2025

On operations like workspace changes, AeroSpace receives a stampede of
accessibility events, each of which triggers a serial call to
refreshSession() on the main thread. Since these refreshSession() calls
are idempotent, we can coalesce these calls so that
multiple accessibility events can be handled by a single
refreshSession().

With this change, operations like workspace switch go from
O(num_source_windows + num_dest_windows) to O(1) refreshSession()
calls (with similar improvements for operations like closing windows or
resizing windows).

@rickyz rickyz force-pushed the coalesce_refresh branch from f87217e to da0cc06 Compare March 30, 2025 20:03
On operations like workspace changes, AeroSpace receives a stampede of
accessibility events, each of which triggers a serial call to
`refreshSession()` on the main thread. Since these `refreshSession()` calls
are idempotent, we can coalesce these calls so that
multiple accessibility events can be handled by a single
`refreshSession()`.

With this change, operations like workspace switch go from
O(num_source_windows + num_dest_windows) to O(1) `refreshSession()`
calls (with similar improvements for operations like closing windows or
resizing windows).
@rickyz rickyz force-pushed the coalesce_refresh branch from da0cc06 to e8e2efa Compare March 30, 2025 20:03
Comment on lines +56 to +59
if screenIsDefinitelyUnlocked {
pendingRefreshScreenIsDefinitelyUnlocked = true
}
return
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if proves that it's not 100% idempotent :/

screenIsDefinitelyUnlocked is definitely a hack anyway that I hope to get rid of in scope #1215 + #1216

Comment on lines +55 to +62
if havePendingRefresh {
if screenIsDefinitelyUnlocked {
pendingRefreshScreenIsDefinitelyUnlocked = true
}
return
}
havePendingRefresh = true
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels more natural to me to coalesce the latest screenIsDefinitelyUnlocked state into pendingRefreshScreenIsDefinitelyUnlocked in whatever case:

Suggested change
if havePendingRefresh {
if screenIsDefinitelyUnlocked {
pendingRefreshScreenIsDefinitelyUnlocked = true
}
return
}
havePendingRefresh = true
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
if havePendingRefresh { return }
havePendingRefresh = true

This way we will run the next refresh session with the latest known screenIsDefinitelyUnlocked

wdyt?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My logic for preferring true here was that the comments at

// Consider the following case:
// 1. Close window
// 2. The previous step lead to caching the whole world
// 3. Change something in the layout
// 4. Lock the screen
// 5. The cache won't be updated because all alive windows are already cached
// 6. Unlock the screen
// 7. The wrong cache is used
//
// That's why we have to reset the cache every time layout changes. The layout can only be changed by running commands
// and with mouse manipulations
@MainActor func resetClosedWindowsCache() {
closedWindowsCache = FrozenWorld(workspaces: [], monitors: [])
}
suggest that resetting the closed window cache on relayout is critical for correctness. Because of that, I was worried that losing a reset via
if screenIsDefinitelyUnlocked { resetClosedWindowsCache() }
could potentially trigger that buggy sequence.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, makes sense.

Merged with the following cleanup patch:

diff --git a/Sources/AppBundle/initAppBundle.swift b/Sources/AppBundle/initAppBundle.swift
index 38751b79..c64d0fc8 100644
--- a/Sources/AppBundle/initAppBundle.swift
+++ b/Sources/AppBundle/initAppBundle.swift
@@ -23,7 +23,7 @@ import Foundation
     AXUIElementSetMessagingTimeout(AXUIElementCreateSystemWide(), 1.0)
     startUnixSocketServer()
     GlobalObserver.initObserver()
-    refreshAndLayout(.startup1, screenIsDefinitelyUnlocked: false, startup: true)
+    refreshSession(.startup1, screenIsDefinitelyUnlocked: false, startup: true, body: {})
     refreshSession(.startup2, screenIsDefinitelyUnlocked: false) {
         if serverArgs.startedAtLogin {
             _ = config.afterLoginCommand.runCmdSeq(.defaultEnv, .emptyStdin)
diff --git a/Sources/AppBundle/layout/refresh.swift b/Sources/AppBundle/layout/refresh.swift
index 8ac24370..91cafb31 100644
--- a/Sources/AppBundle/layout/refresh.swift
+++ b/Sources/AppBundle/layout/refresh.swift
@@ -42,27 +42,16 @@ func refreshSession<T>(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked:
     return result
 }

-@MainActor
-func refreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked: Bool, startup: Bool = false) {
-    refreshSession(event, screenIsDefinitelyUnlocked: screenIsDefinitelyUnlocked, startup: startup, body: {})
-}
-
 @MainActor private var havePendingRefresh = false
-@MainActor private var pendingRefreshScreenIsDefinitelyUnlocked = false

 @MainActor
 func scheduleRefreshAndLayout(_ event: RefreshSessionEvent, screenIsDefinitelyUnlocked: Bool = false) {
-    if havePendingRefresh {
-        if screenIsDefinitelyUnlocked {
-            pendingRefreshScreenIsDefinitelyUnlocked = true
-        }
-        return
-    }
+    if screenIsDefinitelyUnlocked { resetClosedWindowsCache() }
+    if havePendingRefresh { return }
     havePendingRefresh = true
-    pendingRefreshScreenIsDefinitelyUnlocked = screenIsDefinitelyUnlocked
     DispatchQueue.main.async { @MainActor in
         havePendingRefresh = false
-        refreshSession(event, screenIsDefinitelyUnlocked: pendingRefreshScreenIsDefinitelyUnlocked, startup: false, body: {})
+        refreshSession(event, screenIsDefinitelyUnlocked: false, startup: false, body: {})
     }
 }

@nikitabobko nikitabobko added the pr-merged Pull Request is merged label Apr 7, 2025
ZimengXiong pushed a commit to ZimengXiong/winmux that referenced this pull request Apr 6, 2026
On operations like workspace changes, AeroSpace receives a stampede of
accessibility events, each of which triggers a serial call to
`refreshSession()` on the main thread. Since these `refreshSession()` calls
are idempotent, we can coalesce these calls so that
multiple accessibility events can be handled by a single
`refreshSession()`.

With this change, operations like workspace switch go from
O(num_source_windows + num_dest_windows) to O(1) `refreshSession()`
calls (with similar improvements for operations like closing windows or
resizing windows).

closes nikitabobko/AeroSpace#1249
related: nikitabobko/AeroSpace#131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-merged Pull Request is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants