egui icon indicating copy to clipboard operation
egui copied to clipboard

High CPU usage when accesskit enabled

Open sebastiano-barrera opened this issue 1 year ago • 12 comments

Describe the bug

In my app based on egui and eframe, the CPU usage is very high as long as the accesskit feature is enabled. By disabling it, the CPU usage remains under much more acceptable levels, and app itself runs much smoother as well.

(I'm available to work on the bug myself. I'd love some suggestions though; I don't quite see how accesskit/D-Bus traffic could cause this much resource consumption, however much it is.)

To Reproduce Steps to reproduce the behavior:

  1. Build & run the app with the default set of features for both egui and eframe (checkout: https://github.com/sebastiano-barrera/mcjs/commit/b670e5e13534571d73d17bba16eae0a7c4fd3779).
  2. Scroll around while monitoring CPU usage.
  3. Repeat after rebuilding with accesskit disabled (checkout https://github.com/sebastiano-barrera/mcjs/commit/4980384b65ba84c46afd521d02d3adb24cbf31c5).

Expected behavior CPU usage "acceptable" (say, <50%) even while the app is continuously updating (e.g. while scrolling).

Videos

Look at the CPU% column:

Accesskit off, CPU stays < 50%

https://github.com/emilk/egui/assets/7526491/f1844311-f54b-42ca-9ed4-d3c3e2ae1b15

Accesskit on, CPU stays ~200%

https://github.com/emilk/egui/assets/7526491/bb9807d7-1e54-4e68-aa6f-dcd274a4f0de

Desktop:

  • OS: GNU/Linux
  • Version: Fedora Linux 40
  • Desktop environment: GNOME Wayland

Additional context I also have some perf profiles taken that point at a decent % of call stacks involving functions called accesskit::* and zbus::*. It's quite large, so I'll provide it only if you reckon it's useful.

sebastiano-barrera avatar May 22 '24 19:05 sebastiano-barrera

Hello @sebastiano-barrera,

First, I'm assuming you are not running any assistive technology on your system, so AccessKit should not send data through D-Bus. This was fixed in https://github.com/AccessKit/accesskit/issues/324.

Even then, your application likely contain a lot of widgets that should be ignored by assistive technologies. Due to a limitation in AccessKit this used to generate unnecessary D-Bus trafic. This was fixed in https://github.com/AccessKit/accesskit/pull/411.

To see if updating egui's dependency on AccessKit will fix your issue, you can try depending on this fork: https://github.com/mwcampbell/egui/tree/accesskit-0.14-winit-0.29.

Does this help?

DataTriny avatar May 23 '24 22:05 DataTriny

Assuming I patched my Cargo.toml correctly (commit), it seems not to have much of an effect:

Screenshot from 2024-05-24 19-27-24

(The screenshot was taken while doing the same "scrolly" test as in the video in the original post.)

sebastiano-barrera avatar May 24 '24 17:05 sebastiano-barrera

Thanks @sebastiano-barrera for trying my suggestion.

What concerns me here is that recent versions of the accesskit_unix crate include a mechanism to only expose the accessibility tree through D-Bus if it is actually needed and it doesn't seem to work in your case.

Could you please execute this command to see if accessibility is turned on on your system?

busctl --user get-property org.a11y.Bus /org/a11y/bus org.a11y.Status IsEnabled

If you are not running any assistive technology, false should be returned.

I will try running your application on my side as soon as possible to diagnose what is putting so much load onto the system when accessibility is requested.

DataTriny avatar May 24 '24 18:05 DataTriny

Thank you for taking the time to look at this.

The command you suggest does return b false. I don't run any screen reader or other assistive technology that I can think of.

Also, now that you suggest it, if the app's implementation can cause this problem, then there is a good chance that that's what's happening. I'm an egui noob if there ever was one. (In this case, I just wish this was better pointed out in the docs somewhere; maybe that can be my contribution later.)

If you want to try it, it should be sufficient to clone the repo and go with:

cargo run -p mcjs_tools -- ./mcjs_vm/test-resources/json5/test_parse.mjs

Let me know if I can be of any support. This is a hobby project, so unfortunately no thought whatsoever was put into onboarding or user-friendliness...

sebastiano-barrera avatar May 24 '24 20:05 sebastiano-barrera

@sebastiano-barrera Assuming you're still using my accesskit-0.14-winit-0.29 branch of egui, try using cargo update -p accesskit to update the AccessKit dependencies to the latest commit on AccessKit's winit-0.29-backport branch. I can think of one recent AccessKit update that might help.

mwcampbell avatar May 26 '24 11:05 mwcampbell

Hi @sebastiano-barrera,

In the commit you linked above you have updated egui and eframe to their 0.27 version. The proper dependencies are inside the [patch.crates-io] section of your Cargo.toml, which is not used when selecting dependencies on your development environment.

What you need to do is to replace:

egui = "0.27"
eframe = "0.27"

By:

egui = { git = "https://github.com/mwcampbell/egui", branch = "accesskit-0.14-winit-0.29" }
eframe = { git = "https://github.com/mwcampbell/egui", branch = "accesskit-0.14-winit-0.29" }

Then you will need to patch egui_tiles so it uses the same version of egui. I achieved this by cloning egui_tiles, making it depend on @mwcampbell's fork then updating your Cargo.toml to depend on this local version:

egui_tiles = { path = "../../egui_tiles" }

With this I can already see that no D-Bus activity happen when I don't enable any assistive technology and the system load seem to be on par with a build of your app without AccessKit. I would appreciate if you could confirm.

DataTriny avatar May 26 '24 13:05 DataTriny

Done, and confirmed.

(Although, CPU usage reliably hovers around ~60% instead of 40-50% like in the other case.)

I'll admit I'm not so familiar with this mechanism in Cargo, but this surprised me. As far as I can tell, [patch] is the officially recommended mechanism to transitively override dependencies, whereas here we seem to have only overridden the dependency in mcjs_tools and egui_tiles. The result speak for themselves though, so it's clear I have some studying to do.

sebastiano-barrera avatar May 27 '24 16:05 sebastiano-barrera

Happy to hear that @sebastiano-barrera. Just to make sure, the 40-50% CPU load you mention was measured with the patched dependencies?

DataTriny avatar May 27 '24 17:05 DataTriny

The 40-50% CPU load was observed with the first "test commit" I linked (https://github.com/sebastiano-barrera/mcjs/commit/4980384b65ba84c46afd521d02d3adb24cbf31c5).

60% is with the latest attempt like you suggest, with my app's and egui_tiles' Cargo.toml changed "directly" (without [patch]).

Full disclosure, I'm pretty sure the difference is reliable to observe, but I think it's acceptable after all, and I can't rule out a measurement artifact (I'm really just eyeballing htop).

sebastiano-barrera avatar May 27 '24 19:05 sebastiano-barrera

@sebastiano-barrera Assuming you're still using my accesskit-0.14-winit-0.29 branch of egui, try using cargo update -p accesskit to update the AccessKit dependencies to the latest commit on AccessKit's winit-0.29-backport branch. I can think of one recent AccessKit update that might help.

Sorry @mwcampbell, I missed your comment earlier.

Your suggestion pushed me to solve the doubt I had about [patch]: it seems I was right about how it works, but I was wrong about how to use it. I put the [patch] paragraph in the toplevel (workspace) Cargo.toml, not the one for the crate, and the next build got a bunch of package updates:

    Updating git repository `https://github.com/mwcampbell/egui`
    Updating crates.io index
    Updating git repository `https://github.com/AccessKit/accesskit.git`
     Locking 32 packages to latest compatible versions
      Adding accesskit v0.14.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_atspi_common v0.5.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_consumer v0.21.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_macos v0.14.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_unix v0.10.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_windows v0.19.0 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding accesskit_winit v0.20.3 (https://github.com/AccessKit/accesskit.git?branch=winit-0.29-backport#c61d8765)
      Adding ecolor v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding eframe v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding egui v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding egui-wgpu v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding egui-winit v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding egui_glow v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding emath v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding epaint v0.27.2 (https://github.com/mwcampbell/egui?branch=accesskit-0.14-winit-0.29#eb4c9cd5)
      Adding futures-macro v0.3.30
    Updating gpu-descriptor v0.2.4 -> v0.3.0
    Updating gpu-descriptor-types v0.1.2 -> v0.2.0
    Updating image v0.24.9 -> v0.25.1
      Adding immutable-chunkmap v2.0.5
    Updating metal v0.27.0 -> v0.28.0
    Updating naga v0.19.2 -> v0.20.0
    Updating webbrowser v0.8.15 -> v1.0.1
    Updating wgpu v0.19.4 -> v0.20.0
    Updating wgpu-core v0.19.4 -> v0.20.0
    Updating wgpu-hal v0.19.4 -> v0.20.0
    Updating wgpu-types v0.19.2 -> v0.20.0
    Updating windows v0.48.0 -> v0.54.0 (latest: v0.56.0)
      Adding windows-core v0.54.0 (latest: v0.56.0)
    Updating windows-implement v0.48.0 -> v0.53.0 (latest: v0.56.0)
    Updating windows-interface v0.48.0 -> v0.53.0 (latest: v0.56.0)
      Adding windows-result v0.1.1

Judging by the Adding accesskit [...] line, this is equivalent to what you had in mind, right? Commit c61d8765 is the latest, at the time of writing, from the winit-0.29-backport branch.

At any rate, the resulting commit (https://github.com/sebastiano-barrera/mcjs/commit/9e3b23a0e91742639d767f2d7b57b0f3a19d8282) seems to have fixed the issue. With this commit I see 40-50% CPU usage, as good as the "disabled accesskit" commit that I posted initially (https://github.com/sebastiano-barrera/mcjs/commit/4980384b65ba84c46afd521d02d3adb24cbf31c5). This might have "just" been a detour due to my incorrect application of the changes to Cargo.toml.

sebastiano-barrera avatar May 27 '24 19:05 sebastiano-barrera

Oh sorry, I think I misunderstood this package patching feature.

It seem that the correct dependencies are pulled with https://github.com/sebastiano-barrera/mcjs/commit/9e3b23a0e91742639d767f2d7b57b0f3a19d8282.

Should be fixed by #4466 then.

DataTriny avatar May 27 '24 20:05 DataTriny

With #4849 merged, this issue should be solved for everyone.

DataTriny avatar Aug 04 '24 21:08 DataTriny