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 burning CPU with udev disabled on Flatpak #69563

Merged
merged 1 commit into from Dec 5, 2022

Conversation

nyanpasu64
Copy link
Contributor

@nyanpasu64 nyanpasu64 commented Dec 4, 2022

Fixes #67355.
Should be cherry-picked to 3.5.

Note that I haven't tested this fix locally. I'd be happy if someone were to make a no-udev or Flatpak build of 3.5 or master, then test on their machine, and/or publish a Flatpak package (perhaps https://docs.flatpak.org/en/latest/single-file-bundles.html?) so I can test CPU burning (unsure about controllers) my side.

  • Verify CPU burning is fixed
  • Verify startup controllers work
  • Verify controller hotplug works
  • Verify unplugging controllers works(???)

@Riteo
Copy link
Contributor

Riteo commented Dec 4, 2022

I can confirm that after applying this patch an udev-free build doesn't burn a whole core anymore, great work!

@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Dec 5, 2022
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

The fix makes sense to me.

Would be great if someone could test and confirm that controller detection/hotplugging works fine (I don't see why it wouldn't but who knows).

@Calinou
Copy link
Member

Calinou commented Dec 5, 2022

Initial controllers, controller hotplugging and unplugging still work with this PR when building with udev=no. I've tested this on a Wooting two HE (initial controller that is always connected) and a DualSense controller for hotplugging.

Regardless of whether I'm using udev=no or udev=yes, CPU usage remains high in https://github.com/Calinou/godot-demo-projects/tree/update-demos-4.0beta2/misc/joypads (50% of a single core), even though it has low processor mode enabled. This is likely related to something causing continuous redrawing for no reason, though I don't know for certain.

@lawnjelly Do we have any tools to check what causes continuous redrawing in a project, or even a way to check that continuous redrawing is indeed occurring in a project that has low processor mode enabled but no visible animation? (We can't display an animation to check for this, as it'll cause continuous redrawing in itself.)

@akien-mga
Copy link
Member

akien-mga commented Dec 5, 2022

Regardless of whether I'm using udev=no or udev=yes, CPU usage remains high in Calinou/godot-demo-projects@update-demos-4.0beta2/misc/joypads (50% of a single core), even though it has low processor mode enabled. This is likely related to something causing continuous redrawing for no reason, though I don't know for certain.

Note that this demo disables VSync. No idea why. (As a side note, it also has a pointless set_physics_process(true).)

On my system, a nearly empty Godot project seems to use 15-20% CPU (debug build of the editor), both with or without this PR, with or without udev. That's high but likely unrelated to this issue, to be investigated separately

Edit: This 15% CPU usage seems to come from two threads:

  • Main thread: Main::iteration adding a frame delay
  • PulseAudio polling with 1000 µs (2-4% CPU)
    #2  0x0000000006dcefc5 in OS_Unix::delay_usec (this=0x7ffdd61f79e0, p_usec=1000) at drivers/unix/os_unix.cpp:262
    #3  0x0000000006df6321 in AudioDriverPulseAudio::thread_func (p_udata=0x7ffdd61f7bb0) at drivers/pulseaudio/audio_driver_pulseaudio.cpp:570
    

The joypads demo seems to use a lot more CPU indeed but that seems to be specific to that demo's _process method. Adding an early return brings it down to 15-20% like other projects for me. So that demo specifically should be profiled to see why it uses so much CPU (might indeed be related to some Input/joypad methods being called every frame and leading to a lot of cycling around).

@Riteo
Copy link
Contributor

Riteo commented Dec 5, 2022

@akien-mga weird, on my machine (which doesn't have udev at all btw) I can see a considerable improvement even just on the project manager. Without this PR I can see a (process) thread with 100% CPU, while applying it removes said thread completely from the equation. My laptop's also considerably quieter and the average CPU usage is lower.

@lawnjelly
Copy link
Member

@lawnjelly Do we have any tools to check what causes continuous redrawing in a project, or even a way to check that continuous redrawing is indeed occurring in a project that has low processor mode enabled but no visible animation? (We can't display an animation to check for this, as it'll cause continuous redrawing in itself.)

I'm not so familiar with master, but looking at rendering_server_default.h line 101 looks like you can define DEBUG_CHANGES and put a breakpoint in _changes_changed(), and look at the call stack.

@akien-mga
Copy link
Member

This is likely related to something causing continuous redrawing for no reason, though I don't know for certain.

In that demo it might be due to constant text redrawing due to calling set_text and updating the theme on each frame.

@akien-mga akien-mga merged commit 14f23df into godotengine:master Dec 5, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Calinou
Copy link
Member

Calinou commented Dec 5, 2022

Note that this demo disables VSync. No idea why. (As a side note, it also has a pointless set_physics_process(true).)

I disabled V-Sync in the demo to reduce input lag, as low-processor mode takes care of limiting framerate to a reasonable value if something is causing continuous redrawing. It's not very important in the demo, but UI-only apps that don't feature scrolling generally don't benefit from V-Sync in any way.

#66367 would do a better job at limiting FPS to a power-efficient value when V-Sync is disabled, but the default effective FPS cap of 145 isn't too bad otherwise (as it helps bring down latency noticeably on 60 Hz displays compared to a 60 FPS cap without V-Sync).

@nyanpasu64 nyanpasu64 deleted the patch-1 branch December 6, 2022 07:17
@timothyqiu
Copy link
Member

timothyqiu commented Dec 12, 2022

Cherry-picked for 3.6
Cherry-picked for 3.5.2

@timothyqiu timothyqiu removed cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot on Flatpak burns full CPU core scanning for joypads
7 participants