Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix high CPU usage #1481 #1484

Closed
wants to merge 12 commits into from
Closed

fix high CPU usage #1481 #1484

wants to merge 12 commits into from

Conversation

metacodes
Copy link
Contributor

@metacodes metacodes commented Apr 10, 2022

See detail discussion in #1437 .

metacodes and others added 6 commits March 17, 2022 17:33
I am able to reproduce the issue by reinstall JetBrains AppCode. The first time I open AppCode after reinstall it, AltTab never show AppCode window. I debug the code on my Mac, I find that AltTab can get the event of app launched but never get the event of finishedLaunching. So I just remove the code runningApplication.isFinishedLaunching. After that, AltTab work well and the issue is fixed.
workaround: sometimes some apps are launched and are actived but OS returns the windows.count == 0. we wait for a moment. For example: Bear.app. At this scenario, AXUIElementCopyAttributeValue(self, kAXWindowsAttribute, &value) returns success but  AXUIElementCopyAttributeValue(self, kAXPositionAttribute, &value) and AXUIElementCopyAttributeValue(self, kAXSizeAttribute, &value) return attributeUnsupported. After we wait for a moment, all things run well.
@metacodes metacodes changed the title fix issue #1481 fix high CPU usage #1481 Apr 10, 2022
We use  `isManageable` to indicate the subscription will success or not. No need to subscribe to an app which is `LSBackgroundOnly` or `LSUIElement`.
self.isReallyFinishedLaunching = true
self.observeNewWindows()
// we only need to subscribe to those apps which will have windows
if AXUIElement.isManageable(runningApplication) {
Copy link
Owner

@lwouis lwouis Apr 11, 2022

Choose a reason for hiding this comment

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

This check is a bit late. Better not even create the Application instance, if we think it will not spawn any window. There is already such a method to filter apps here:

private static func isActualApplication(_ app: NSRunningApplication) -> Bool {

You could add your new filtering logic there.

That being said, see the comment in the method I linked. Some apps actually change the value of their activationPolicy later after they launch. So today we observe activationPolicy and if it changes, we subscribe to the app's events. Maybe we would need to do the same thing with things like LSUIElement. Not sure. Maybe a first implementation could be to check at startup, and later if people complain and we find specific app switch during their lifetime, then we would listen to those flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I found a compensation method to solve this problem. The initial technical attempts have been made, and I'll implement the code for this later. I'm a bit busy these two weeks.

Comment on lines 316 to 321
if bundleInfoDictionary["LSBackgroundOnly"] != nil {
return true
}
if bundleInfoDictionary["LSUIElement"] != nil {
return true
}
Copy link
Owner

Choose a reason for hiding this comment

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

If I remember correctly, checking for these is redundant with activationPolicy. I think activationPolicy is simply a modern/simplied/swift variable which apple exposes on top of the old ones.

I may be wrong though. I would be nice to confirm that by printing it for a few apps and confirming that in some cases these can disagree. That would mean we should check all of those. If they always agree, then this is not helping

* @return true if the application will have windows and false otherwise.
*/
static func isManageable(_ runningApp: NSRunningApplication) -> Bool {
guard let bundleIdentifier = runningApp.bundleIdentifier else {
Copy link
Owner

Choose a reason for hiding this comment

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

we have cases like the android emulator where the app has no bundleIdentifier but is still app with windows (see

if app.bundleIdentifier == nil,
)

Comment on lines 252 to 270
"com.apple.dashboard",
"com.apple.loginwindow",
"com.apple.notificationcenterui",
"com.apple.wifi.WiFiAgent",
"com.apple.Spotlight",
"com.apple.systemuiserver",
"com.apple.dock",
"com.apple.AirPlayUIAgent",
"com.apple.dock.extra",
"com.apple.PowerChime",
"com.apple.WebKit.Networking",
"com.apple.WebKit.WebContent",
"com.apple.WebKit.GPU",
"com.apple.FollowUpUI",
"com.apple.controlcenter",
"com.apple.SoftwareUpdateNotificationManager",
"com.apple.TextInputMenuAgent",
"com.apple.TextInputSwitcher"
])
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it's a good idea to hardcode specific bundleIdentifiers. If these start creating windows in further macOS releases, then AltTab will have to be updated to show them. Also how did you build that list? Maybe it's missing processes from previous macOS versions you don't use, or maybe (actually it's certain) different processes will run on your mac depending on your System Preferences, if you have a network printer, external monitor, keyboard, if your phone does AirPlay with the mac, etc.

I think we should find more flags or properties of apps that give strong hints that they will not spawn windows. Also retrying less often than 10ms is already a large workaround the CPU issue

@@ -21,7 +21,7 @@ func retryAxCallUntilTimeout_(_ group: DispatchGroup?, _ timeoutInSeconds: Doubl
} catch {
let timePassedInSeconds = Double(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) / 1_000_000_000
if timePassedInSeconds < timeoutInSeconds {
BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(10)) {
BackgroundWork.axCallsQueue.asyncAfter(deadline: .now() + .milliseconds(1000)) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think a value of 250ms may be a better comprise: 10ms is very aggressive for a responsive UX but at the cost of CPU. 1s is easy on the CPU, but a bit laggy. I think 250ms is similar to human delay in processing changes on screen. So maybe a better compromise. See https://humanbenchmark.com/tests/reactiontime

@metacodes
Copy link
Contributor Author

metacodes commented Apr 12, 2022

@lwouis I use new method to aovid subcribing to applications without windows. See details in the code comments. Currently testing is OK. Need more testing.

@@ -104,7 +105,30 @@ class Applications {
private static func isActualApplication(_ app: NSRunningApplication) -> Bool {
// an app can start with .activationPolicy == .prohibited, then transition to != .prohibited later
// an app can be both activationPolicy == .accessory and XPC (e.g. com.apple.dock.etci)
return (isNotXpc(app) || isAndroidEmulator(app)) && !app.processIdentifier.isZombie()
return (isNotXpc(app) || isAndroidEmulator(app)) && !app.processIdentifier.isZombie() && isAnWindowApplication(app)
Copy link
Owner

Choose a reason for hiding this comment

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

If your comment below is correct (that apps that trigger a didActivateApplicationNotification event must have windows), then all these checks are no longer useful for these apps, no?

I imagine that they would only be useful for the wasLaunchedBeforeAltTab == true apps. So maybe we should add that boolean check first (left-side), to avoid the more expansive checks in the case where it's wasLaunchedBeforeAltTab == false, no?

// I'm not very sure if there is an edge case, but so far testing down the line has not revealed it.
// When we receive the didActivateApplicationNotification notification, NSRunningApplication.isActive=true, even if the app is not the frontmost window anymore.
// If we go to add the application when we receive the message of apps launched, at this time NSRunningApplication.isActive may be false, and try axUiElement.windows() may also throw an exception.
// For those background applications, we don't receive notifications of didActivateApplicationNotification until they have their own window. For example, those menu bar applications.
Copy link
Owner

Choose a reason for hiding this comment

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

If this is true, then your approach here to listen to .didActivateApplicationNotification is genius! Did you think or it yourself or was it used by other projects?

Also, if I summarize this change, it seems you found an API that gives us properly filtered apps (only apps with windows), but that's only for apps launched after AltTab is started. For apps launched before AltTab is started, we still have to filter out processes with activationPolicy, XPC, etc. But luckily, apps launched before AltTab should not throw an exception on .windows().

And then you mention the edge-case of an app launching at the same time as AltTab, which would be annoying. This is actually a very important edge-case, unfortunately. That's what happens when you log into macOS. All the startup apps launch at once, concurrently. This means AltTab is launched, and all sorts of app at the very same time.

I guess for that scenario, didActivateApplicationNotification will not help us, and we will need to rely on the other existing checks (XPC, etc), and with the new 250ms delay, it may be an acceptable situation?

Am I understanding correctly the whole approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you think or it yourself or was it used by other projects?

At first I was thinking that when we open an application, as long as they show a window, then a change in window switching is bound to happen, and if we can find a way to observe this change, we will be able to tell that the program is equipped with a window. Then I used some window management software, such as automatic window layout APP, like AppGrid, Rectangle, and found that they can observe this change very well, so I went to study their code, and then found in Rectangle's code that it subscribes to .didActivateApplicationNotification notifications. I was curious, so I wrote some test code to see the effect. Turns out it worked and was able to solve half of the problem, the other half still had to be figured out.

And then you mention the edge-case of an app launching at the same time as AltTab, which would be annoying. This is actually a very important edge-case, unfortunately. That's what happens when you log into macOS. All the startup apps launch at once, concurrently. This means AltTab is launched, and all sorts of app at the very same time.

That's my concern, and I still need to do some testing on this situation.

I guess for that scenario, didActivateApplicationNotification will not help us, and we will need to rely on the other existing checks (XPC, etc), and with the new 250ms delay, it may be an acceptable situation?

This edge case only happens when wasLaunchedBeforeAltTab=true, at which point we go to the axUiElement.windows() test. If it happens that the program takes a long time to load, then axUiElement.windows() will most likely throw an exception, and at that point we do need to detect it by other means (e.g. XPC, etc), otherwise we will lose the window. Of course, reopening the program that lost the window, AltTab will capture its window without any problem.

Copy link
Contributor Author

@metacodes metacodes Apr 13, 2022

Choose a reason for hiding this comment

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

For that edge-case, we probably don't need to restart the applications that are missing windows, we can do a check when we get the .didActivateApplicationNotification notification, and if the application is not in the AltTab list of existing windows, then we add it. We will receive a .didActivateApplicationNotification notification every time a window switch occurs between different applications.

@metacodes
Copy link
Contributor Author

@lwouis I have submitted a new commit for fixing #1324 and some minor improvements to full-screen thumbnails.

Comment on lines +127 to +133
guard let winApp = (allWindows.first { app.processIdentifier == $0.ownerPID()
&& $0.isNotMenubarOrOthers()
&& $0.id() != nil
&& $0.bounds() != nil
&& CGRect(dictionaryRepresentation: $0.bounds()!)!.width > 0
&& CGRect(dictionaryRepresentation: $0.bounds()!)!.height > 0
}) else {
Copy link
Owner

Choose a reason for hiding this comment

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

As I said in the other ticket, you're re-implementing a mirror to

static func isActualWindow(_ runningApp: NSRunningApplication, _ wid: CGWindowID, _ level: CGWindowLevel, _ title: String?, _ subrole: String?, _ role: String?, _ size: CGSize?) -> Bool {
here. This will not be good enough. AltTab will thus show windows it shouldn't show.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add these codes just to filter those apps which have no windows(e.g. com.apple.universalcontroller). This is for those apps launched before AltTab. At that time, we haven't go to AXUIElement.isActualWindow. The code is just there to allow us to avoid observing to applications we are not interested in and to avoid wasting CPU time on subscribeToNotification retries.

@_silgen_name("CGSAddWindowsToSpaces")
func CGSAddWindowsToSpaces(_ cid: CGSConnectionID, _ windows: NSArray, _ spaces: NSArray) -> Void

// Move the given windows (CGWindowIDs) to the given space (CGSSpaceID)
// * macOS 10.10+
@_silgen_name("CGSMoveWindowsToManagedSpace")
Copy link
Owner

Choose a reason for hiding this comment

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

Does this work exactly like CGSAddWindowsToSpaces? I remember testing almost every API from https://github.com/phracker/MacOSX-SDKs/blob/10dd4868459aed5c4e6a0f8c9db51e20a5677a6b/MacOSX10.10.sdk/System/Library/Frameworks/CoreGraphics.framework/Versions/A/CoreGraphics.tbd which had the word "space" or "window" in it. I'm surprised that I missed this one. Are you sure it works in every scenario (e.g. multiple monitors, minimized window, hidden app, etc)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This api will move the window from the original space to another space. It means the window will disappear from the original space and show on another space. I did not test the scenarios you mentioned. I just use this api to move the invisible window to the space we want to. I will do some tests. By the way, I can not do the tests for multiple monitors, because I only have one monitor.😂

@@ -122,10 +122,15 @@ func CGSCopyWindowsWithOptionsAndTags(_ cid: CGSConnectionID, _ owner: UInt32, _
func CGSManagedDisplayGetCurrentSpace(_ cid: CGSConnectionID, _ displayUuid: CFString) -> CGSSpaceID

// adds the provided windows to the provided spaces
// * macOS 10.10+
// * macOS 10.10-12.2
Copy link
Owner

Choose a reason for hiding this comment

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

RIP 😄

@@ -19,11 +19,63 @@ extension CGWindowID {
return CGSCopySpacesForWindows(cgsMainConnectionId, CGSSpaceMask.all.rawValue, [self] as CFArray) as! [CGSSpaceID]
}

func screenshot() -> CGImage? {
// CGSHWCaptureWindowList
// fullscreen has multiple windows
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand this. I just fullscreen'ed Notes.app, and AltTab shows the same thing I see.

Could you share screenshots to illustrate what you mean by "toolbar window and a main window", and generally what problem this new method fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before adding the fullScreenshot method
image

After adding the fullScreenshot method
image

Maybe it depends on machine model. I'm using MBP 16' with Apple Silicon.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, it does it on my machine too (still on Catalina). Very cool then! I wonder how many apps really benefit from that nuance though. It's a cool trick, but in the case of Notes.app, it's not very necessary. Maybe some other apps have an important UI element that would get lost though. I worry a bit about the complexity/performance/potential-failure-points of created the composition vs showing the simple image, though. I think it would probably be worth it to first find an app that truly benefits from this refinement before putting it in. What do you think? Especially given that in order to do this trick, we have to rely on CGS API, which has all the problems I mentioned in other comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I also took into account the performance issues. In fact, this small issue has little impact, I just made it a little bit optimized as free time. Reminders.app, Hopper Disassembler.app, Xcode.app, Microsoft Word.app, Calendar.app also have this problem as I known.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I tried to use CGWindowListCreateImageFromArray, but it didn't work at all. Maybe I'm not using it the right way.

self.size = bounds!.size
self.position = bounds!.origin
updatesWindowSpace()
if CGSSpaceGetType(cgsMainConnectionId, self.spaceId) == .fullscreen {
Copy link
Owner

Choose a reason for hiding this comment

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

Another worry with using CGS APIs instead of AX APIs is that I don't know if there are delays when calling it. AX has lots of problems like it will block for a long time if the app is busy or the system is busy. We have systems to retry on a loop to eventually get the info. It tooks years and is pretty elaborate and deals with all sorts of edge-cases. Here we are making a simple/naive call to some CGS API. We are making the call on the main thread, which means that any delay from that API will make AltTab UI stutter/freeze. The AX calls are always happening on another thread so that AltTab never freezes. It's a nightmare of concurrent programming but it was the only way to get rid of lag/freeze/unresponsiveness, and it's the reason AltTab is (mostly) very responsive, regardless of how laggy the OS is at responding to its APIs.

thumbnail = NSImage(cgImage: cgImage, size: NSSize(width: cgImage.width, height: cgImage.height))
thumbnailFullSize = thumbnail!.size
}

func close() {
if isWindowlessApp { return }
if isWindowlessApp || self.axUiElement == nil { return }
Copy link
Owner

Choose a reason for hiding this comment

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

Does this mean we can't close a window if we haven't visited its Space? That not ideal...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a pity that HyperSwitch also can't do that either.

Copy link
Owner

Choose a reason for hiding this comment

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

I just tested and it can.

I did this to test:

  • Opened a new window
  • Send it to another Space
  • Launch HyperSwitch
  • Press alt-tab
  • Notice that HS shows the window
  • Hover the mouse over the window in HS
  • A close button appears, press it
  • The window on the other Space is closer, even though I never navigated to that Space

How can HS close the window without the AX reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On macOS 12.3.1, I did the steps same to you, but HyperSwitch just took me to that space and didn't close the window. Maybe the private APIs they used also broken.

@@ -102,7 +140,7 @@ class Window {
}

func minDemin() {
if isWindowlessApp { return }
if isWindowlessApp || self.axUiElement == nil { return }
Copy link
Owner

Choose a reason for hiding this comment

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

See comment above

@@ -119,7 +157,7 @@ class Window {
}

func toggleFullscreen() {
if isWindowlessApp { return }
if isWindowlessApp || self.axUiElement == nil { return }
Copy link
Owner

Choose a reason for hiding this comment

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

See comment above

self.axUiElement.focusWindow()
}
}
} catch {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think exceptions can bubble up to here. retryAxCallUntilTimeout happens on another thread, and absorbs exceptions within

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, during the testing process, it was found that this happens. Because of the animation of the space switch, it causes us not to get the axUiElement?.windows() right away, so we need to retry several times.

Applications.observeNewWindowsBlocking()
CGSRemoveWindowsFromSpaces(cgsMainConnectionId, windowsOnlyOnOtherSpaces as NSArray, [Spaces.currentSpaceId])
// Currently we add those window in other space without AXUIElement init
// We don't need to get the AXUIElement until we focus these windows.
Copy link
Owner

Choose a reason for hiding this comment

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

If you're correct that CGSMoveWindowsToManagedSpace is a perfect replacement to CGSAddWindowsToSpaces, then we could at least fix the 12.3 issue with CGSAddWindowsToSpaces not working, but replacing the call to CGSMoveWindowsToManagedSpace. That would fix the issue. Then, separately, we can have the conversation about the invisible window trick, and how to move forward there

Copy link
Owner

Choose a reason for hiding this comment

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

Please see my follow-up here: #1324 (comment)

@lwouis
Copy link
Owner

lwouis commented Apr 28, 2022

FYI, I just pushed a release with these 2 commits: 0569ed0 cdb461a

I thought they may mitigate the high CPU issues people are facing at the moment. I think this buys more time for this pull-request to become more robust and maybe deliver breakthroughs

# Conflicts:
#	src/api-wrappers/AXUIElement.swift
@lwouis
Copy link
Owner

lwouis commented Oct 14, 2022

@metacodes I'll close this PR now since I merged part of it. If you're still interested in improving perf, could you please open a new PR with your new improvements?

@lwouis lwouis closed this Oct 14, 2022
@metacodes
Copy link
Contributor Author

@metacodes I'll close this PR now since I merged part of it. If you're still interested in improving perf, could you please open a new PR with your new improvements?

OK👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants