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

High CPU usage when accesskit enabled #4527

Open
sebastiano-barrera opened this issue May 22, 2024 · 12 comments
Open

High CPU usage when accesskit enabled #4527

sebastiano-barrera opened this issue May 22, 2024 · 12 comments
Labels
accessibility More accessible to e.g. the visually impaired bug Something is broken performance Lower CPU/GPU usage (optimize)

Comments

@sebastiano-barrera
Copy link

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: sebastiano-barrera/mcjs@b670e5e).
  2. Scroll around while monitoring CPU usage.
  3. Repeat after rebuilding with accesskit disabled (checkout sebastiano-barrera/mcjs@4980384).

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%

accesskit.off.cpu.usage.lower.mp4

Accesskit on, CPU stays ~200%

accesskit.on.cpu.usage.high.mp4

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 sebastiano-barrera added the bug Something is broken label May 22, 2024
@emilk emilk added accessibility More accessible to e.g. the visually impaired performance Lower CPU/GPU usage (optimize) labels May 23, 2024
@DataTriny
Copy link
Contributor

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 AccessKit/accesskit#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 AccessKit/accesskit#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?

@sebastiano-barrera
Copy link
Author

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.)

@DataTriny
Copy link
Contributor

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.

@sebastiano-barrera
Copy link
Author

sebastiano-barrera commented May 24, 2024

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

@mwcampbell
Copy link
Contributor

@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.

@DataTriny
Copy link
Contributor

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.

@sebastiano-barrera
Copy link
Author

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.

@DataTriny
Copy link
Contributor

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

@sebastiano-barrera
Copy link
Author

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

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
Copy link
Author

sebastiano-barrera commented May 27, 2024

@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 (sebastiano-barrera/mcjs@9e3b23a) 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 (sebastiano-barrera/mcjs@4980384). This might have "just" been a detour due to my incorrect application of the changes to Cargo.toml.

@DataTriny
Copy link
Contributor

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

It seem that the correct dependencies are pulled with sebastiano-barrera/mcjs@9e3b23a.

Should be fixed by #4466 then.

@DataTriny
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility More accessible to e.g. the visually impaired bug Something is broken performance Lower CPU/GPU usage (optimize)
Projects
None yet
Development

No branches or pull requests

4 participants