macOS: QuickTerminal under certain circumstances spins at 100% CPU

[mitchellh]

Discussed in #3723

^{Originally posted by stuartcarnie December 28, 2024}
Noticed that Ghostty was using 100% CPU, so I grabbed a CPU profile under Instruments and found that the hottest call stack was related to the Quick Terminal feature:

When I drill into that stack, QuickTerminalController.syncAppearance is a hot function. I currently don't have the window open, and I believe the culprit is this code:

ghostty/macos/Sources/Features/QuickTerminal/QuickTerminalController.swift

Lines 310 to 318 in 196af6f

	// If our window is not visible, then delay this. This is possible specifically
	// during state restoration but probably in other scenarios as well. To delay,
	// we just loop directly on the dispatch queue. We have to delay because some
	// APIs such as window blur have no effect unless the window is visible.
	guard window.isVisible else {
	// Weak window so that if the window changes or is destroyed we aren't holding a ref
	DispatchQueue.main.async { [weak self] in self?.syncAppearance(config) }
	return
	}

Given the window isn't open, this will repeatedly call the syncAppearance function.

[!NOTE]

Re-opening the Quick Terminal reduces the CPU to nominal levels again.

To re-trigger the high CPU usage, force a reload of your configuration and watch it spike again.

[mitchellh]

We should flag a pending sync appearance and wait for the window to become visible (via notifications) to apply it.

[divs1502]

Hi, I was just visiting through the code trying to understand the bug what I believe we should check again if self is still valid before calling syncAppearance

DispatchQueue.main.async { [weak self] in if let self = self { self.syncAppearance(config) } }

[mitchellh]

That would probably work but if we can avoid queueing up multiple that'd be ideal, so I think we should fix this in a more robust way.

[divs1502]

Yes, correct then using the flag to prevent queuing multiple sync calls and avoiding unnecessary CPU usage spikes would be ideal.

[dmehala]

I observed this behavior and called it here #3742, however I didn't thought it was that bad and was hoping to get some feedback to address it directly in the PR. However, @mitchellh, I can understand if you prefer to address the performance issue separately.

EDIT: Well, I opened #4055.

[pijvari]

Hi, I was just visiting through the code trying to understand the bug what I believe we should check again if self is still valid before calling syncAppearance

DispatchQueue.main.async { [weak self] in if let self = self { self.syncAppearance(config) } }

Hmm, does this code change any behaviour? as the original code DispatchQueue.main.async { [weak self] in self?.syncAppearance(config) } have optional chaining in self?.syncAppearance... it wouldn't call syncAppearance anyway if self is nil?

[divs1502]

Hi, I was just visiting through the code trying to understand the bug what I believe we should check again if self is still valid before calling syncAppearance
DispatchQueue.main.async { [weak self] in if let self = self { self.syncAppearance(config) } }

Hmm, does this code change any behaviour? as the original code DispatchQueue.main.async { [weak self] in self?.syncAppearance(config) } have optional chaining in self?.syncAppearance... it wouldn't call syncAppearance anyway if self is nil?

Yes you are correct although this does not explicitly change any code behavior but this will help maintain readability and avoid multiple calls ( if we need to perform multiple actions that depend on self, using if let self avoids repeating self? ).