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

Merged

Conversation

Thomas-Vos
Copy link
Contributor

@Thomas-Vos Thomas-Vos commented Dec 9, 2024

My app is used a lot in the background, and I noticed high CPU usage in the native macOS version. With this PR, CPU usage is reduced when window is invisible by disabling drawing. This was already implemented for the JVM, and that behaviour is now copied to native macOS. So the code is similar to JVM parts in:

The draw function is now suspend, which required moving some other functions around. I tried to keep the implementation similar to the JVM version.

Testing

Tested using ./gradlew runNative in samples/SkiaMultiplatformSample. Now the CPU usage is reduced a lot when moving another (non transparent) app over the window.

Old behaviour

Screen.Recording.2024-12-09.at.17.23.28.mov

New behaviour with 300ms timeout

See reduction in CPU usage when window is invisible

Screen.Recording.2024-12-09.at.17.26.17.mov

New behaviour without 300ms timeout

Screen.Recording.2024-12-11.at.23.27.20.mov


// 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

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.

@MatkovIvan MatkovIvan requested a review from igordmn December 9, 2024 16:55
@Thomas-Vos
Copy link
Contributor Author

Thomas-Vos commented Jan 29, 2025

@igordmn it would be great if this could be reviewed. This PR improves CPU usage for native macOS but also JVM macOS.

@igordmn igordmn requested a review from m-sasha January 29, 2025 19:12
@igordmn igordmn self-assigned this Jan 31, 2025
@m-sasha
Copy link
Member

m-sasha commented Feb 5, 2025

Seems that the change @igordmn mentioned is the only one here for JVM.

I think it's a bit risky, but I'm not opposed to it. One thing we should do though, is to run all Compose tests with this change, before merging. @Thomas-Vos Could you do that?

@m-sasha
Copy link
Member

m-sasha commented Feb 5, 2025

Just FYI, I noticed a strange (existing) behavior in my app. Sometimes if I run it and immediately switch to a different window before the app's window is shown, it will never get shown. I haven't investigated it, though, so it may or may not be related.

@Thomas-Vos Thomas-Vos force-pushed the fix-high-cpu-usage-window-occluded branch from 806b7a8 to d435e84 Compare February 8, 2025 02:41
@Thomas-Vos
Copy link
Contributor Author

@m-sasha rebased and I ran the tests on latest compose-multiplatform-core in :compose only for macosArm64, some tests were already failing (19) even without this PR. After including Skiko local build I also got 19 failed tests. Not sure what to do about this.

I could not find any information on how to set up the compose-multiplatform repo with a local build of compose-multiplatform-core and skiko. So unfortunately I was not able to test that repo.

If it's too risky I can take out the change and put it in a separate PR. Then at least the macOS Native changes could get merged.

Screenshot 2025-02-08 at 03 47 28

@m-sasha
Copy link
Member

m-sasha commented Feb 8, 2025

@m-sasha rebased and I ran the tests on latest compose-multiplatform-core in :compose only for macosArm64, some tests were already failing (19) even without this PR. After including Skiko local build I also got 19 failed tests. Not sure what to do about this.

@Thomas-Vos Let me run the tests myself first then.

@igordmn
Copy link
Collaborator

igordmn commented Feb 13, 2025

If we are not sure and it requires tests, let's just revert the jvm change and add withTimeout to macOs too.

Then there will be no obstacles for merging, the change will be simple.

@m-sasha
Copy link
Member

m-sasha commented Feb 13, 2025

If we are not sure and it requires tests, let's just revert the jvm change and add withTimeout to macOs too.

I think that's best. I'm not 100% convinced that occluded windows should not recompose anyway.

@Thomas-Vos Thomas-Vos changed the title Fix high CPU usage when window is occluded on macOS Fix high CPU usage when window is occluded on macOS Native Feb 14, 2025
@Thomas-Vos
Copy link
Contributor Author

@igordmn reverted the change, thanks for the review.

@igordmn
Copy link
Collaborator

igordmn commented Feb 14, 2025

Thanks!

Also removed Release Notes.

@igordmn igordmn merged commit 852c96b into JetBrains:master Feb 14, 2025
6 checks passed
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