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.

Sign up for GitHub

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 when window is occluded on macOS Native #1003

Merged
merged 3 commits into from
Feb 14, 2025
Merged

Fix high CPU usage when window is occluded on macOS Native #1003

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b41c8c
Fix high CPU usage when window is occluded on macOS
Thomas-Vos Dec 9, 2024
d435e84
Remove 300ms timeout
Thomas-Vos Dec 11, 2024
97c7d21
Revert "Remove 300ms timeout"
Thomas-Vos Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions skiko/src/macosMain/kotlin/org/jetbrains/skiko/redrawer/MetalRedrawer.macos.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ import kotlinx.cinterop.autoreleasepool
import kotlinx.cinterop.objcPtr
import kotlinx.cinterop.usePinned
import kotlinx.cinterop.useContents
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.withTimeoutOrNull
import org.jetbrains.skia.BackendRenderTarget
import org.jetbrains.skia.DirectContext
import org.jetbrains.skiko.FrameDispatcher
import org.jetbrains.skiko.SkikoDispatchers
import org.jetbrains.skiko.SkiaLayer
import org.jetbrains.skiko.context.ContextHandler
import org.jetbrains.skiko.context.MacOsMetalContextHandler
import platform.AppKit.NSWindowDidChangeOcclusionStateNotification
import platform.AppKit.NSWindowOcclusionStateVisible
import platform.CoreGraphics.CGColorCreate
import platform.CoreGraphics.CGColorSpaceCreateDeviceRGB
import platform.CoreGraphics.CGContextRef
Expand All @@ -26,6 +30,11 @@ import platform.QuartzCore.kCALayerHeightSizable
import platform.QuartzCore.kCALayerWidthSizable
import kotlin.system.getTimeNanos
import platform.CoreGraphics.CGSizeMake
import platform.Foundation.NSNotification
import platform.Foundation.NSNotificationCenter
import platform.Foundation.NSOperationQueue
import platform.darwin.NSObjectProtocol
import kotlin.concurrent.Volatile

/**
* Metal [Redrawer] implementation for MacOs.
Expand All @@ -43,9 +52,24 @@ internal class MacOsMetalRedrawer(
private val queue = device.newCommandQueue() ?: throw IllegalStateException("Couldn't create Metal command queue")
private var currentDrawable: CAMetalDrawableProtocol? = null
private val metalLayer = MetalLayer()
private val occlusionObserver: NSObjectProtocol
private val windowOcclusionStateChannel = Channel<Boolean>(Channel.CONFLATED)
@Volatile private var isWindowOccluded = false

init {
metalLayer.init(skiaLayer, contextHandler, device)

val window = skiaLayer.nsView.window!!
occlusionObserver = NSNotificationCenter.defaultCenter.addObserverForName(
name = NSWindowDidChangeOcclusionStateNotification,
`object` = window,
queue = NSOperationQueue.mainQueue,
usingBlock = { notification: NSNotification? ->
val isOccluded = window.occlusionState and NSWindowOcclusionStateVisible == 0uL
isWindowOccluded = isOccluded
windowOcclusionStateChannel.trySend(isOccluded)
}
)
}

private val frameDispatcher = FrameDispatcher(SkikoDispatchers.Main) {
Expand All @@ -72,6 +96,7 @@ internal class MacOsMetalRedrawer(
override fun dispose() {
if (!isDisposed) {
metalLayer.dispose()
NSNotificationCenter.defaultCenter.removeObserver(occlusionObserver)
isDisposed = true
}
}
Expand Down Expand Up @@ -115,17 +140,30 @@ internal class MacOsMetalRedrawer(
*/
override fun redrawImmediately() {
check(!isDisposed) { "MetalRedrawer is disposed" }
draw()
autoreleasepool {
if (!isDisposed) {
skiaLayer.update(getTimeNanos())
contextHandler.draw()
}
}
}

private fun draw() {
// TODO: maybe make flush async as in JVM version.
private suspend fun draw() {
autoreleasepool {
if (!isDisposed) {
skiaLayer.update(getTimeNanos())
contextHandler.draw()
}
}

// When window is not visible - it doesn't make sense to redraw fast to avoid battery drain.
if (isWindowOccluded) {
withTimeoutOrNull(300) {
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 copied this 300 millisecond value from the JVM code to keep it consistent. However, I don't understand the reason for this delay, why not just suspend indefinitely (on native & jvm)? I can remove this timeout if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some testing it seems all works fine without the 300ms timeout. CPU usage is reduced even more, on both native and JVM. Added a screenrecording with results for native app in the PR. I could not find any specific reason this timeout was added in git blame, so I removed it. If there's something I am missing please let me know.

What still stands out to me is that CPU and memory usage are both much lower for native vs JVM on my M1 Max macbook, with the same FPS result (100fps, my refresh rate). This is the also the case for my own much heavier app.

Copy link
Collaborator

@igordmn igordmn Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Rendering each 300 ms was added before windowOcclusionStateChannel.receive() is introduced, to avoid full CPU loading, because a window in occluded state isn't synchronised with the display refresh rate (we had 1000 FPS).

After windowOcclusionStateChannel.receive() was added, the 300 ms just wasn't removed.

Everything will work as before if an app rendering logic doesn't contain any side effects besides just changing visuals. A hypothetical side effect in Compose could be some action in response to a state change inside a window's composition:

  • send a system notification
  • show a modal dialog
  • send analytics

It is not correct to write such actions inside a window composition, because they don't affect window's visuals. It is fine if they are called outside of window composition (in LaunchedEffect, or in the application composition).

Because it is incorrect, we can do this change.

But we should add Release notes in the PR description:

## Release Notes (general)
### Breaking changes - Desktop
- rendering on macOs is no longer executed if a window is occluded. Some applications can stop working correctly if they contain non window-related side effects inside rendering logic.

## Release Notes (Compose)
### Breaking changes - Desktop
- rendering on macOs is no longer executed if a window is occluded. Some applications can stop working correctly if they contain non window-related side effects inside composition/layout/draw

@m-sasha, could you review the JVM change as well? It looks fine by me, but if you have doubts, we can reduce the scope of the PR, by keeping only the macos native changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igordmn thanks for the explanation! I added the release notes to the description.

// If the window becomes non-occluded, stop waiting immediately
@Suppress("ControlFlowWithEmptyBody")
while (windowOcclusionStateChannel.receive()) { }
}
}
}

fun finishFrame() {
Expand Down
Loading