Implementation discussion to reduce freezes due to RPC calls

[raisjn]

I've been trying out different methods for reducing freezes and would like to catalog them. so far, I don't think there is a single solution that will make @nikitabobko happy but would like to share my experiences.

Approaches

Approach 1: Put all window updates into a single thread. Accomplished by placing detectNewWindowsAndAttachThemToWorkspaces into its own DispatchQueue and run it on a loop with a thread timeout of 50ms to 100ms.

Approach 2: Put each window's updates into its own separate queue and run them on a while loop with sleep.

Approach 3: Put detectNewWindowsAndAttachThemToWorkspaces call into a separate queue when called from refreshSession.

Approach 4: Put detectNewWindowsAndAttachThemToWorkspaces call into a separate queue when called from refreshSession and wait up to X ms for it to complete.

Approach 5: Put detectNewWindowsAndAttachThemToWorkspaces call into a debouncer and a separate queue when called from refreshSession and wait for up to X ms for it to complete. Also place validateStillPopups behind a similar debouncer + dispatch queue.

EDIT: 12/05: ported perf patch to latest origin/main branch

Approach 6: Put refreshSession in a debouncer
Approach 6a: Put refreshSession in a debouncer and attempt to correctly reconcile body effects

Note: Thread safety: for detectNewWindowsAndAttachThemToWorkspace to be threadsafe, MacWindow.get() has an NSLock placed in it so that multiple threads can access it safely. Additionally, specific work within MacWindow.get() should be moved to the main thread using a DispatchQueue

Other Changes I've tried

• Cache monitors so they aren't re-calculated frequently when calling sortedMonitors
• Remove menu items from the menubar that involve retrieving the workspace (since I don't use the menubar to retrieve workspaces)
• Debounce refreshSession - I think this causes more bugs than it solves

Observed Bugs:

These are misc. bugs I've observed. In the future, I will put them into a table to say which bugs appear on which approach.

Bug A: Starting a popup window and dragging it across workspaces doesn't work (for example, System Settings). The window will get stuck on a single workspace and keep hopping back. Dragging the window within a single workspace works fine. It is possible to move the window via keyboard shortcuts, though. (I think this happens on all approaches? I haven't tested stock Aerospace)

Bug B: Focus follows mouse (AutoRaise application) doesn't play well with aerospace and multiple monitors. Switching the workspace on one monitor (monitor 1), then moving to another monitor (monitor 2) with the mouse will often cause the workspace on monitor 1 to switch back to the previous workspace. (I think this happens on all approaches? I haven't tested stock Aerospace)

Bug C: When throwing a window from one workspace to another, the window will not retile itself until the next input command from aerospace. This would effectively leave a window unresized until a button combination causes Aerospace to refresh layout. This only happens with some approaches.

Bug D: Performance degrades over time - after several days, switching between workspaces slows down when I run a CPU stress app. It makes me think there is work that happens per window or some growing set of resources that are being iterated through. It is somewhat more difficult to debug because starting a fresh debug session (with extra info) will have no lag, but going back to the original session (long running) will continue to lag when under CPU duress
(Update 11/08): After debugging for several days, also added a debouncer for validateStillPopups that may help with the performance degradation over time.

(Update 11/07) Bug E: I have recently (in the past day or two) run into the errorT from Window.parent. This is unexpected and then went away - I am not sure what was causing it. I had rebased off main branch and found it causes more errors in general, so I rolled back to an earlier version. I pushed a change to mask this error for now.

Usage (Update 11/09)

To use the dev/okay/2024_11_02_less_cpu_lockups branch, recompile the debug or release build and add the following lines to your aerospace.toml:

new-window-detection-timeout = 100

@liamaharon suggests the following may have better results:

new-window-detection-timeout = 50
new-window-detection-debounce = 100

What it does: new-window-detection-timeout is how long to block the main thread (up to X ms), while new-window-detection-debounce will coalesce calls within X ms together.

[raisjn]

Thanks to @raisjn for doing the performance experiments. The debouncing approach makes sense. The refreshSession may be called too often when a lot of things happen on the screen (e.g. when you switch the workspace), and given the idempotent nature of refreshSession, debouncing the whole session becomes trivial and may increase the app responsiveness. I don't agree with the specifics of implementation, but the general idea makes sense, thanks for your experiments. I will consider "the debouncer" approach when start working on performance.

@nikitabobko: note my comment about debouncing refreshSession having some small bugs in it. if you can get it working, that is great! overall, I don't think solely debouncing refreshSession will be enough to stop hangs: if any app misbehaves, you will eat the 1s timeout (per app) and that is already too much delay.

PS: iiuc, refreshSession is idempotent if no body is passed in, but when a body is passed in, then it is executing code that has side effects.

As correctly pointed by @raisjn, please discuss the details of @raisjn fix in #655

if you can re-parent comments from #131 here, that would be great

[nikitabobko]

Sorry for the late response, I was on a vacation

note my comment about debouncing refreshSession having some small bugs in it

If you describe what bugs you observed, maybe I can quickly tell you what was wrong. But I think you will rather have more bugs (especially concurrent bugs) if you try to debounce only the part of the refreshSession

PS: iiuc, refreshSession is idempotent if no body is passed in, but when a body is passed in, then it is executing code that has side effects.

Yes

I don't think solely debouncing refreshSession will be enough to stop hangs: if any app misbehaves, you will eat the 1s timeout (per app) and that is already too much delay.

Yes, #131 (comment)

if you can re-parent comments from #131 here, that would be great

GitHub doesn't allow it it. It's only possible to convert the whole issue to the discussion

[liamaharon]

fyi @raisjn (and anyone else who may be experiencing this issue)

new-window-detection-timeout = 50
new-window-detection-debounce = 100

on dev/okay/2024_11_02_less_cpu_lockups is working wonderfully for me in initial experiments. Much appreciated ❤️

[raisjn]

awesome, really happy that these hacks helped someone else! I pushed another commit that may help with perf degradation over time - if you notice perf degradation, please try it out.

[vianneychin]

I'd love to try your build! Stupid question, but what would the step by step look like to install the forked version? I normally get it from homebrew.

[wojciech-kulik]

Download repo, run bundle install, run ./install-from-sources.sh. I had to remove also from build-shell-completions.sh the following line and commit changes locally:

fish -c 'source ./.shell-completion/fish/aerospace.fish'

[raisjn]

thanks @wojciech-kulik!

just FYI: I've ported the patch on a new branch that is based on current origin/main