kikipoulet / SukiUI

UI Theme for AvaloniaUI
MIT License
1.51k stars 140 forks source link

High CPU usage issues #167

Open Cardroid opened 7 months ago

Cardroid commented 7 months ago

Describe the bug I'm currently working on a program that draws audio plots. I'm aware of the high CPU utilization because of the work of drawing cool animations.

However, the program seems to be constantly drawing new frames on a still screen (even when there is no user input). CPU seems to be being wasted unnecessarily due to continuous frame refreshes. How can we improve it?

To Reproduce Steps to reproduce the behavior:

  1. Apply the Suki library and run the program.

Expected behavior CPU utilization should be close to 0% on a still screen without events

Screenshots without SukiUI low cpu CPU usage: 0% ~ 1%

with SukiUI high cpu CPU usage: 5% ~ 6%

Environment

The plotting library used ScottPlot

kikipoulet commented 7 months ago

Hi,

Unfortunately I noticed this behavior a few times ago. The problem is that it does not seem to be one thing in particular that trigger this, but the overall complexity of the logical tree. It's not just removing the dynamic background, or the dialog grid, that can solve this, everything that we use "add" a little complexity and it ends at 4-5%, even on my machine.

Well, it was my observation at least. But performance is a feature, and I kept that problem in my mind, it does not seem normal anyway. I think you maybe found more with the rendering observations, so if I summary well, SukiUI suddenly renders insane amount of frame there and there when normally it would cap at 30 fps ?

Moreover, can you try to use SukiUI theme without using SukiWindow to see what it produces ? just to see if it changes something, I suspect that the SukiWindow is the source of 90% of the performance problem.

It would be great if we find a way to fix this actually 👍

Cardroid commented 7 months ago

Thank you for your answer. 👍

I also want to continue using this wonderful library. After the development of the core BM, I will investigate it.

This is just my guess... I think it's related to background animation. (when I see a lot of GC calls at the beginning of the program running)

kikipoulet commented 7 months ago

Thanks for your kind words.

I don't think it's related to the background animation because it's just at startup, and removing it doesn't change anything, but maybe the fps mystery is related

kikipoulet commented 7 months ago

Just did a few test. The weird thing is that if you create a new project and add just one button without even suing SukiWindow, the CPU usage will already be +/- 3%. So the +/-5% of the SukiDemo project is quite normal after seeing that. It just seems that the existence of SukiUI trigger this 😶‍🌫️

I wonder if it's about how styles or resources are declared or included ? Maybe there is a more optimized way ?

Can you confirm that just using a button in a blank window produce +/-3% CPU too ?

CyLuGh commented 7 months ago

My guess would be the BlurBackground.

I’ve just done a search in code to see if there was any ICustomDrawOperation. I used this in the past to draw a component with skia, and it also resulted in such CPU usage. It does not seem to execute only when the component is invalidated, it’s constantly being called to draw.

CyLuGh commented 7 months ago

The work around I found at the time is here: https://github.com/CyLuGh/HierarchyGrid/blob/main/src/HierarchyGrid.Avalonia/SKXamlCanvas.axaml.cs

It's a control that behaves mostly like the WPF SKCanvas. If I manage to find some time, I'll see if it solves the problem here, but you're free to try if I'm too slow ;)

kikipoulet commented 7 months ago

The BlurBackground isn't use anymore because it wasn't as efficient as I wanted it to be. So basically the file can be completely removed, and I've just tested SukiDemo whith it removed it logically doesn't change anything, so unfortunately it's not the reason.

The weird thing is that just using a button in a "normal avalonia" window already trigger 3-4%.

kikipoulet commented 7 months ago

I did few tests. It's interesting to create a simple window with a button and switch from simpletheme to SukiTheme in App.axaml without changing anything. The Suki variant needs more CPU, +/- 1.5 while SimpleTheme needs +/- 0.5%.

Just for a button. I noticed that few details could be optimized. I probably should remove some boxshadow in some controls because removing shadow from the button free a little percentage. Removing the custom font release some, removing cornerradius too .. Actually every single thing adds a little percentage.

So, it's more a bad news actually, because there is no miracle. At this point I think there are 3 things that could help :

kikipoulet commented 6 months ago

For reference/reminder, slight performance optimisation could be achieved by using more CompositionAnimations instead of transitions, like this example for GlassCard :

animation

The main problem is that not everything can be animated with CompositionAnimations, but it is reasonable to think about :

Cardroid commented 6 months ago

It's good to see it improving little by little. 👍

I'm also attempting to trace the cause of this issue, but it seems like high CPU utilization and redrawing the frame are other issues.

As you mentioned, the complexity of the visual tree seems to be the main culprit for increasing CPU utilization. (Even if I removed the controls that I estimated would be heavy, such as background controls, etc., from the demo project, the same results were seen.) Unfortunately, I couldn't find where the trigger to redraw the frame was coming from. Tracking stack traces didn't provide any useful clues. 🥲

sirdoombox commented 5 months ago

I've spent a little time looking into this as well, took a variety of snapshots in dotTrace and nothing jumps out at me from our end specific to anything we've implemented. Even after the inclusion of the ICustomDrawOperation needed to render the new dynamic background, I noticed little to no increase in CPU usage.

The only thing I have noticed is that WindowImpl.AppWndProc seems to pop up with a fairly high time, fairly often. That would generally point to an overly complex visual tree and inefficient messaging though it doesn't necessarily confirm either.

I'm reasonably concerned about this, we haven't really noticed any decreases in performance recently so there has to be some fundamental issues that are hanging around but it's hard to track down what precisely. I think the most productive use of time would be to try and convert every existing control and style to use the ControlTheme to minimise the need for selectors to be so widely used and flatten the visual tree wherever it's possible as well as investigating any alternative more efficient ways to deal with property change behaviours instead of using observables (though I believe this is the accepted way).

Major things like not hosting everything inside the SukiHost and simply having it draw on top may help, though it might be an idea to look at something a bit more mature like Material.Avalonia to see how they've implemented things as they have a large number of fairly complex controls and styles yet perform far better out of the box.

Maruhl commented 5 months ago

Hi, I on the other hand don't have an unusual CPU load but more GPU load.

I open the demo and the GPU load jumps from 2%->6~7%. Something is constantly triggered to repaint.

grafik

I then looked further for fun, and ended up with "SukiBackground". If I comment out the code in the "SukiWindow" - the GPU load still remains low. I also don't know if there are different loads for the users due to different CPU's and GPU's. Maybe I am completely wrong. I just wanted to throw it into the room.

sirdoombox commented 5 months ago

Yep GPU utilisation for the SukiBackground is much more preferable to CPU load, it was a conscious choice to start offloading some of the heavier visual aspects off to something much more designed for it. The GPU scales much, much better for things like this and leveraging hardware acceleration I believe was the right choice.

Really our only focus should be on the CPU and getting that as low as possible. There are probably optimisations that will improve GPU utilisation in SkiaSharp 3 but I'm not that concerned. SukiUI is very visually rich and as long as the appropriate hardware is being used to do a lot of heavy lifting then I'm a lot more comfortable with that personally.

I have gotten SukiUI running with SkiaSharp 3 preview locally and there's no major improvements, seems to be a fairly minor bump in fps but GPU/CPU utilisation seems to be mostly stable.

wslsj888 commented 2 months ago

sukitest <suki:SukiWindow xmlns="https://github.com/avaloniaui" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:d="http://schemas.microsoft.com/expression/blend/2008" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" mc:Ignorable="d" d:DesignWidth="800" d:DesignHeight="450" xmlns:suki="https://github.com/kikipoulet/SukiUI" x:Class="STC.Suki.MainWindow" Title="STC.Suki"> Welcome to Avalonia! </suki:SukiWindow> An empty application,only use SukiWindow,CPU usage exceeds 30%, GPU usage exceeds 40%。…(⊙_⊙;)…

ShawnXu97 commented 1 month ago

@kikipoulet I think i find what ploblem,but i'm not sure changing that will making another issues, I try it by myself the Cpu and Gpu nearly 0. SukiUI.Controls.SukiBackground in this file Render function is over and over to call Dispatcher.UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background),if return before the issues will gone! So is there any reason to call it ? if use DispatcherTimer to call it with 1s is great? 1728899717961 image

kikipoulet commented 1 month ago

Hi @ShawnXu97 , This is a great discovery ! Thank you very much for your work.

Actually, removing the call doesn't seem to change anything, except for the "animated background" functionality I think. @sirdoombox do you confirm ? Maybe we should just call the function when AnimationEnabled property is True ? Or I completely miss it ? During my test I don't notice any changes when removing it.

kikipoulet commented 1 month ago

I notice that the loading control have an incomplete animation if removing this, it seems to be the only problem.

ShawnXu97 commented 1 month ago

@kikipoulet I'm happy it helped~ about animation doesn't work maybe can try to add some Timer refresh the call to active it, I think the call is keep to draw new frame .but that will be low performance,maybe rebuild a new loading controll is better?

kikipoulet commented 1 month ago

@kikipoulet I'm happy it helped~ about animation doesn't work maybe can try to add some Timer refresh the call to active it, I think the call is keep to draw new frame .but that will be low performance,maybe rebuild a new loading controll is better?

Initially, SkiaSharp was used to draw our custom background. I guess this line is for the "Animated Background" option that is not a big problem to drop or render like that only in this case. It's a feature not even referenced in our documentation because honestly not a lot of peoples will use it.

However, we recently used SkiaSharp too for the loading control because of a memory leak bug when using Inifinite Animation in Avalonia ( https://github.com/AvaloniaUI/Avalonia/issues/17192 ) which, by the way, is quite a problem for the framework.

But I'm not worried, the consistent CPU % leak was more a problem than a loading control, we'll just wait for sirdoombox who knows this part way better than me to see if there is a way to render loading at 60fps only, and correct the background to render only when a style change occurs, or if we look for another solution for the loading control to let the background render only at style changes and solve this issue.

sirdoombox commented 1 month ago

I've taken a quick look at this and indeed the invalidate visual call is a bit of a heavy hitter, unfortunately I can't find a way around it. The animations are entirely reliant on the invalidation of the visual. Trying to disable the invalidation when animations are disabled also requires a separate call to the UI thread and actually performs worse.

I've tried a few things including changes to the priority and running a separate task at a much higher interval but it bumps into the same UI thread issue and ends up performing worse.

I feel we're a sticky situation where any animations reliant on ICustomDrawOperation require a full repaint, even in the case of the loading icon. This kinda sucks and I'm not familiar enough with the particulars of Avalonia's underlying rendering code to see any obvious alternatives, especially now as we're reliant on it for our loading icons to prevent a memory leak.

The only solution I see to this in the short term is finding a way to render the loading icon in a traditional way and completely removing animated effects from SukiUI. However there may be some obvious thing I'm missing which is causing such a noticeable hit to performance.

(I will add a sidenote that as it stands I only see rare peaks of 5%-6% CPU usage and mostly hovering in the ~2% area in the demo app.)

ShawnXu97 commented 1 month ago

@sirdoombox Cpu using isn't the worse,When i use it the Gpu will 20%~30% on some device , I know the device is not very well but if i want to use it ,this is a big ploblem.

I'm try debug the code,I think the call making Render is a loop ,the render isn't finish, the next render call is running , Is there any way to make the Render only render it self?

PS: In this way i think it will refresh the whole window.

sirdoombox commented 1 month ago

The Render call in ICustomDrawOperation should wait for the GPU to finish it's frame, as the SkiaSharp call to draw the rect with the shader is a blocking one, it also relies on managed memory so that holds. I just double checked and I get ~20% GPU utilisation on a 4070 super and ~5% CPU as-is. Removing the InvalidateVisual call whilst still rendering the frames drops that down to ~3% GPU and ~1% CPU. (Not a perfect test, I have other things running on my system)

Unfortunately the problem is solely the InvalidateVisual call to update animations, to my knowledge there is no way around that and my attempts to run a Task which calls InvalidateVisual less frequently actually resulted in worse performance, not better.

Dispatcher.UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background); switching the priorities for this also doesn't seem to help at all, and it's called pretty much as fast as your system can handle no matter what.

Unless there's something obvious I'm missing, Animation support will necessarily come with a pretty hefty performance hit because of that invalidate visual call, and any attempted solutions to disable the invalidate visual call when animations are disabled causes worse performance again as a separate UI thread invocation to grab the state of the AnimationEnabled property is a big hit.

The only solution to this particular performance woe is removal of animations entirely for the background, and of course a new approach to Loading that doesn't rely on it.

kikipoulet commented 1 month ago

The only solution to this particular performance woe is removal of animations entirely for the background, and of course a new approach to Loading that doesn't rely on it.

Completely agree, thanks for your time and sorry for the skia loading control we're already dropping 🫣

We can then disable skiasharp animations because of performance priority, disable the background animation in SukiDemo, and I'll just find a new trick for a loading control, I'm not worried about that.

@ShawnXu97 if you want you can make the PR that just comment the InvalidateVisual call as you found it, otherwise I'll do it later.

sirdoombox commented 1 month ago

I will investigate SkiaSharp 3.0 as well if I get some time and go a bit more in depth to see if there are any improvements there, though I doubt it.

I am happy to either strip out animations entirely from the underlying API or simply add some extra steps to allow people to force them on if they are happy to eat the performance cost (and I'll put this in the docs too) - it will also allow us to keep trying fixes and track improvements going forward.

kikipoulet commented 1 month ago

We must be careful with SkiaSharp 3.0 because when I tried it it was incompatible with some important libraries like LiveCharts that a lot of peoples (and I) use that are still on 2.8, it's kind of annoying

sirdoombox commented 1 month ago

I certainly wouldn't bring it into production but it would be nice to know if it's an issue that will eventually be solved or if it's a limit in Avalonia, would help plan around the feature for the future.

I should have some time maybe this evening to fix up the underlying API to make animations a bit harder to enable with more warnings, I might also look to switch to attached properties for the background because duplicating properties in SukiWIndow is obviously causing maintenance headaches now.

Gillibald commented 1 month ago

https://github.com/AvaloniaUI/Avalonia/discussions/16357

ShawnXu97 commented 1 month ago

@sirdoombox @kikipoulet Unfortunately, we were working in the wrong direction from the beginning. After re reading the code, I found that ShaderToyrenderer and Loading are both inherited from Avalonia We don't need to deal with the issue of dynamic background anymore, just remove Dispatcher UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background); And by continuously calling the InvalidateVisual() method in ShaderToyrenderer and Loading, all issues were resolved!

I didn't submit the PR directly because the method I wrote may not be very good, and I need to determine whether it is in rendering, otherwise it should not be called again, so I still need to trouble the two of you to handle it.

image

kikipoulet commented 1 month ago

@sirdoombox @kikipoulet Unfortunately, we were working in the wrong direction from the beginning. After re reading the code, I found that ShaderToyrenderer and Loading are both inherited from Avalonia We don't need to deal with the issue of dynamic background anymore, just remove Dispatcher UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background); And by continuously calling the InvalidateVisual() method in ShaderToyrenderer and Loading, all issues were resolved!

I didn't submit the PR directly because the method I wrote may not be very good, and I need to determine whether it is in rendering, otherwise it should not be called again, so I still need to trouble the two of you to handle it.

It seems to move the problem. As a loading control is embedded in every Button control, the same behavior will happen in every page with a button.

InvalidateVisual in Loading :

{6B660261-6BE7-4BD8-82D6-B5F88E894C1F}

Invalidate Visual disabled :

{C8A53FEF-EAA0-4674-AFB1-9BA332E31D9D}

However, it's normal that disabling the animation use less CPU, it's just that it seems a little excessive and maybe a pure avalonia animation will cost less, or maybe embed it better in the button. I will take a look and compare it with Avalonia animations to have more informations to base our decision on.

maxkatz6 commented 1 month ago

@sirdoombox @kikipoulet @sirdoombox I am not familiar with exact implementation of rendering effects for SukiUI, but I might be able to share some useful information.

  1. Don't use Dispatcher UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background); to trigger next render. It's an old approach, which tied to dispatcher timings. Also overloads dispatcher itself. Instead, use topLevel.RequestAnimationFrame API, where you can call InvalidateVisual from the callback.

  2. Generally speaking, any custom rendering should do as much work as possible on Render thread, and as less as possible on UI thread. Control.Render method is executed on the UI thread, where it collects drawing operations. ICustomDrawOperation.Draw is executed on the Render thread.

  3. ICustomDrawOperation is good for custom rendering, but it still requires manual invalidation on UI thread, as well as re-trigger of Render method. Potentially invalidating way too much. In 11.0 we have a new API to solve this problem - CompositionCustomVisual. We have a sample here. The main difference is that you register this custom visual once when control is attached to the tree. And optionally you send sync-messages via visual.SendHandlerMessage. CompositionCustomVisualHandler then can render itself and invalidate itself, if necessary, without even touching UI thread.

And of course, if re-triggering rendering is not necessary, it's much desired to avoid invalidating anything.

kikipoulet commented 1 month ago

So I compared with a loading control animated with Avalonia Animationand it seems to be quite comparable with skiasharp implementation :

{309E4CC8-C338-44F3-9B96-0895D959C32F}

So I guess the plan can be :

Edit : fixed button/busyarea loading control in fae64e5438bae870766902f9777768f9a2e1cd93, now no unnecessary loading control is attached and processing until loading property is true. Still need to make a clean skiasharp implementation or fallback to simple avalonia animation as no memory leak (https://github.com/kikipoulet/SukiUI/issues/245) will happen unless genereting thousand of loading buttons.

sirdoombox commented 1 month ago

@sirdoombox @kikipoulet @sirdoombox I am not familiar with exact implementation of rendering effects for SukiUI, but I might be able to share some useful information.

1. Don't use `Dispatcher UIThread.InvokeAsync(InvalidateVisual, DispatcherPriority.Background);` to trigger next render. It's an old approach, which tied to dispatcher timings. Also overloads dispatcher itself. Instead, use `topLevel.RequestAnimationFrame` API, where you can call InvalidateVisual from the callback.

2. Generally speaking, any custom rendering should do as much work as possible on Render thread, and as less as possible on UI thread. `Control.Render` method is executed on the UI thread, where it collects drawing operations. `ICustomDrawOperation.Draw` is executed on the Render thread.

3. `ICustomDrawOperation` is good for custom rendering, but it still requires manual invalidation on UI thread, as well as re-trigger of Render method. Potentially invalidating way too much. In 11.0 we have a new API to solve this problem - `CompositionCustomVisual`. We have a [sample here](https://github.com/AvaloniaUI/Avalonia/blob/3e3d434cf229a3f4dd0d18b1ba4bd5b72b1d41a6/samples/ControlCatalog/Pages/CompositionPage.axaml.cs#L193-L316). The main difference is that you register this custom visual once when control is [attached to the tree](https://github.com/AvaloniaUI/Avalonia/blob/3e3d434cf229a3f4dd0d18b1ba4bd5b72b1d41a6/samples/ControlCatalog/Pages/CompositionPage.axaml.cs#L203-L210). And optionally you send sync-messages via `visual.SendHandlerMessage`. `CompositionCustomVisualHandler` then can render itself and invalidate itself, if necessary, without even touching UI thread.

And of course, if re-triggering rendering is not necessary, it's much desired to avoid invalidating anything.

@maxkatz6

Utilising the topLevel.RequestAnimationFrame for invalidating the visual had a minor but not insignificant improvement when it came to performance, the problem seems to be that InvalidateVIsual call and without it the background simply doesn't update unless another control on top of it invalidates. My understanding is that the Render method of ICustomDrawOperation isn't called until the invalidation happens. Obviously any time Invalidate is called the GPU gets hit relatively hard.

I've taken a quick look at the CompositionCustomVisual sample you provided and whilst it's a little confusing I broadly see how it works, however I'm concerned that the end result would still require a per-frame InvalidateVisual to actually cause the background to update though I'm not sure about that.

I feel as if I'm getting a bit out of my wheelhouse here, rendering is not really something I'm super familiar with.

maxkatz6 commented 1 month ago

however I'm concerned that the end result would still require a per-frame InvalidateVisual to actually cause the background to update though I'm not sure about that.

If you need your background to continuously render, i.e. animating, then yes - you will need to invalidate it each frame. But the deal is how you invalidate that.

Normal Control.InvalidateVisual() invalidates UI control, potentially causing whole app to re-collect render draw operations (i.e. call Control.Render() method everywhere, just in case if any of these controls overlap). But if you call CompositionCustomVisualHandler.Invalidate(), from the inside of the render thread, this thread will keep the same scene graph of draw operations from the previous frame, while invalidating operations of this particular composition visual.

It still will re-draw the app, but this time all work will be done on Render thread only, reusing most of the operations from previous frames.

maxkatz6 commented 1 month ago

It still will re-draw the app

Note, it's forced by modern GPU based rendering APIs, like DX11/OpenGL, where it's easier to re-draw whole scene, then re-draw particular rectangle. But we do support the latter for embedded devices with framebuffer rendering.

sirdoombox commented 1 month ago

I quickly switched over to CompositionCustomVisual this morning to see if it would have any major effect on performance but unfortunately it made no difference whatsoever - though the ability to easily control invalidation really helped with preventing Invalidate calls when animations were disabled.

The underlying Invalidate call is just a killer for us, there's no getting away from that 4-5% CPU and 20-30% GPU utilisation it seems like, even when I only use canvas.Clear(SKColors.Gray); so that none of our effects are running and nothing actuallly changes between each frame, the GPU usage remains equally high because of the Invalidate call. It's also telling that even when running a relatively complex shader on top on the ShaderToy style Effects page, GPU usage remains high, but completely flat.

I think animated backgrounds are pretty much an impossibility in Avalonia at the moment, the need to redraw the entire window when a control at the very back covering the entire window is invalidated is just too big of a performance hit, 20% utilisation on a high end modern GPU isn't really reasonable for a simple visual feature on a desktop app.

I'll rewrite everything internally to use CompositionCustomVisual and retain the ability to use animations, but I will mark it with warnings both in XMLDocs and in our other documentation that it's highly experimental and comes with a huge performance cost.

sirdoombox commented 1 month ago

This should be largely resolved at this point. Obviously enabling animations still has a pretty big impact on performance but the default state of SukiUI should now be significantly more performant with the switch to CompositionCustomVisual allowing us to only invalidate what we need when we need.

The only outstanding issues are that the Loading control still uses animated effects and causes some performance issues.

I think the issue should stay open until Loading is dealt with and we get some follow ups from people confirming that the performance is more in line with what they are expecting.