Fixes to accelerate and avoid processsor hogging in QSProcessMonitor.m: #3087
Conversation
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
- Only include app processes in list, exclude XPC and background - Avoid calling the slow [NSBundle bundleWithIdentifier:]
Member
|
Thanks for the PR! A few points of clarification:
I am hesitant to merge a potential fix until we have some clues about the last point here. |
Author
|
Thanks!
Why don't you give this PR a try in your local copy? You should notice in debug output that the time spent in |
Member
|
Awesome, thanks for your response!
Glad to hear it. I'm optimistic about the future of LLMs as an adjunct, but my personal experience with them so far has been somewhere between comedic and horrifying, at least as far as coding goes. (Your code is very well commented, which seems to be a hallmark of LLM code in other FOSS projects I'm involved in, which is what prompted the question.)
I'm actually a physician, coding is self-taught and just a hobby! I imagine your ObjC is likely far better than mine; for QS I've mostly been helpful for CI processes and a tiny bit of Swift. Yes, you're certainly not the only one who's experienced this or a similar bug, I'd just like to understand it a bit more thoroughly. For example, over the last several minutes, my running QS has only reloaded processes a few times: This is while I'm actively using the computer (including QS), I have over 80 tabs open between two Firefox windows, 2 xcode processes building, 18 tabs in zellij (with 2-3 panes per tab), nix cross-compiling something in the background... a lot going on. Why is QS reloading so frequently for you, but not for me? |
Member
|
@pjrobertson I see Is there some implicit behavior somewhere? |
Author
|
Thanks @n8henrie ! Yes - I found the experience of understanding the QS code a little frustrating because the original code had so few comments, so I made sure to add more ;-) I used to code quite a bit of Objective-C (as a hobbyist) 15-20 years ago, so I still remember a bit... Why QS is reloading so frequently for me: I think it's primarily due to the specific Safari circumstance (maybe bug) that respawns content service worker processes at high frequency (not constantly, but from time to time, in bursts). If you're using FF instead of Safari, that may already explain. In any case, I guess it can be safe to assume that in the wild there are many systems with frequent process respawns, and this shouldn't trigger QS going to its knees. And also, in any case, XPC/background processes should not burden QS unless the user has explicitly set the "include background processes" toggle. My patch fixes all of that. On |
Member
I agree that this is probably not uncommon, and the fact that it doesn't seem to frequently be bringing QS to its knees is why I'm interested. What about your environment is provoking the problem, whereas others have no such issue? I have I added little println debugging, and it does look like web processes are the most common cause for a reload. I'd like to narrow things down to a reproducible case. |
Author
|
Yes - web worker processes are probably the most frequent trigger. And, at least on my system, with Safari the frequency can get really high. You're seeing these bursts yourself in the log you provided - processes being spawned within milliseconds. On my system, this can go on for minutes. May be a function of the web sites I have open. Finding the root cause of this will be elusive, I believe. QS should just be more robust against this scenario. |
Member
I agree. Verifying robustness will require reproduction! I think I figured out a hacky method. Initially just tried making a little app bundle and launching it in a loop, but nothing happened. However, in that list I noticed a smallish Rust app I recently discovered (Ferrite), so I just tried launching it in a loop (with a very short timeout), and it caused a slight slowdown in QS. Was wondering if this might be a good opportunity to just have some kind of configurable timeout for the reloadApps. Users not experiencing ill effects can continue to have near-instant updates, those experiencing issues could set it to once per second or what have you. Will try to nail down this repro and build your branch and go from there. Thanks again for the dialog! |
Author
|
Yep - I agree, throttling could be yet another level of protection. This could even be automatic, without requiring configuration. But I guess that for most practical purposes, my fix will already go a long way. Resources are probably spent better elsewhere IMO. |
Member
|
Using this as a small test case (and adding a little extra logging to QS at the import AppKit
final class AppDelegate: NSObject, NSApplicationDelegate {
func applicationDidFinishLaunching(_ notification: Notification) {
print("Hello World (launch)")
NSApp.terminate(nil)
}
func applicationWillTerminate(_ notification: Notification) {
print("Goodbye World (terminate)")
}
}
let app = NSApplication.shared
app.setActivationPolicy(.regular)
let delegate = AppDelegate()
app.delegate = delegate
app.run()I notice that even with extremely fast restarts (here 0.01), I get minimal lag with QS as is (but I do see my logging about Can you confirm that this test case provokes severely degraded performance for you? With your PR, I get no lag whatsoever. Unfortunately I also get no reloading and no notification of this process launching and terminating (even with background procs enabled). I would think that command line tools should be fair game to prompt a refresh of the running process list, so I don't think we should merge as-is. Will continue thinking of modifications that would help it resolve the issue for you but keep existing behavior for those that want it. Glad to have found a POC that we can test with! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This is an attempt at fixing #3070.
- (BOOL)includeApp:(NSRunningApplication *)appfor this purpose. The logic is inspired by (but does not copy 100% the logic inappIsBackground:inQuicksilver/Quicksilver/Code-QuickStepFoundation/NSWorkspace_BLTRExtensions.m
Line 193 in 310597e
[NSBundle bundleWithIdentifier:]