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.

Sign up for GitHub

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

Jump to bottom

perf(x11): use a Timer instead of a DispatcherTimer for render callbacks #19054

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

perf(x11): use a Timer instead of a DispatcherTimer for render callbacks #19054

wants to merge 3 commits into from
+92 −47

Conversation

ramezgerges
Copy link
Contributor

GitHub Issue (If applicable): closes #19042

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@ramezgerges ramezgerges changed the title chore(x11): use a Timer instead of a DispatcherTimer for render callbacks perf(x11): use a Timer instead of a DispatcherTimer for render callbacks Dec 10, 2024
@ramezgerges
Copy link
Contributor Author

@HakanL With this PR, the Uno app shouldn't be scheduled by the OS at all if there's nothing to do and should not be spin-waiting at all. Let us know if this fixes the problem.

jeromelaban
jeromelaban reviewed Dec 10, 2024
View reviewed changes
src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs Outdated
// running XConfigureEvent and rendering callbacks on a timer is necessary to avoid freezing in certain situations
// where we get spammed with these events. Most notably, when resizing a window by dragging an edge, we'll get spammed
// with XConfigureEvents.
_timer = new Timer(static obj =>
Copy link
Member

Choose a reason for hiding this comment

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

Could we conditionally stop the timer if we know we do not have anything to draw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't directly pause the System.Threading.Timer but I think there are ways to get around this, if we're careful enough. However, I don't think it matters. The Timer actually sleeps (i.e. the equivalent of a Thread.Sleep) so waking up and immediately sleeping again 60 times a second should be more than fine.

Copy link
Member

@jeromelaban jeromelaban Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The issue here is about CPU idle time, which may prevent CPU to go in low-power modes. It's generally best to avoid doing work we know nothing needs to be done.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19054/index.html

@HakanL
Copy link

HakanL commented Dec 10, 2024

@ramezgerges I'd be happy to test this on my RPi once you think it's ready to try. I haven't run my application with non-Nuget Uno (from source), do you have any docs on how to do that? I think last time I tried I wasn't able to successfully build it, but it was a while ago (may have been missing dependencies for Android, Mac, etc, since I'm only doing Win and Linux).

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19054/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19054/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19054/index.html

@unodevops
Copy link
Contributor

⚠️⚠️ The build 149972 has failed on Uno.UI - CI.

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-19054/index.html

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-19054/index.html

@ramezgerges
Copy link
Contributor Author

ramezgerges commented Dec 16, 2024 •
edited
Loading

@ramezgerges I'd be happy to test this on my RPi once you think it's ready to try. I haven't run my application with non-Nuget Uno (from source), do you have any docs on how to do that? I think last time I tried I wasn't able to successfully build it, but it was a while ago (may have been missing dependencies for Android, Mac, etc, since I'm only doing Win and Linux).

The CI for each PR generates nuget packages that can be used in a local feed. To test this PR, download the nuget packages from here, extract the zip, create a new entry in NuGet.Config and point to the extracted zip's directory, change all references of Uno.Sdk to Uno.Sdk.Private, and change the Uno.Sdk.Private version to the version found in the nupkg names (5.6.0-PullRequest-19054-150374-19054.1190). Let us know if it works out.

@unodevops
Copy link
Contributor

⚠️⚠️ The build 150374 has failed on Uno.UI - CI.

jeromelaban
jeromelaban reviewed Dec 16, 2024
View reviewed changes
src/Uno.UI.Runtime.Skia.X11/X11XamlRootHost.cs
Comment on lines +562 to +563
_renderTimer.Interval = TimeSpan.FromTicks(delta);
_renderTimer.Start();
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, we're setting up the interval to be the time since the last update? Aren't we trying to render immediately, or at least at the FPS interval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're rendering immediately if the last frame was more than 1/60 secs ago. If the last frame is within the last 1/60 secs, we wait the remainder of this 1/60 secs "time slice" before rendering again, basically capping the frame rate at 60.

@HakanL
Copy link

HakanL commented Dec 16, 2024

@ramezgerges I'd be happy to test this on my RPi once you think it's ready to try. I haven't run my application with non-Nuget Uno (from source), do you have any docs on how to do that? I think last time I tried I wasn't able to successfully build it, but it was a while ago (may have been missing dependencies for Android, Mac, etc, since I'm only doing Win and Linux).

The CI for each PR generates nuget packages that can be used in a local feed. To test this PR, download the nuget packages from here, extract the zip, create a new entry in NuGet.Config and point to the extracted zip's directory, change all references of Uno.Sdk to Uno.Sdk.Private, and change the Uno.Sdk.Private version to the version found in the nupkg names (5.6.0-PullRequest-19054-150374-19054.1190). Let us know if it works out.

Is that the correct artifact URL? It only downloads two binlogs in the zip file.

@jeromelaban
Copy link
Member

@HakanL use the artifacts from here:
image

@HakanL
Copy link

HakanL commented Dec 16, 2024

That worked better, thanks @ramezgerges ! I'll grab them and test locally.

@HakanL
Copy link

HakanL commented Dec 16, 2024

Alright, I was able to run with the updated Nuget packets. So far it's looking very promising, I used to be around 45-50% CPU time when idling (as viewed from top, I assume that is one CPU pegged 100% as the RPi is Quad core).
After using this PR I'm at around 25-30%, which is a great improvement! I'm still a little curious why it's still this high vs <1% on my PC, but obviously not an apples to apples comparison. Still a great improvement that I'll take any day of the week! My vote is for merging this.

@HakanL
Copy link

HakanL commented Dec 17, 2024

@ramezgerges I'm getting this error while trying to build in Release. I've tried to set the Uno.Toolkit.WinUI and Uno.Toolkit.Skia.WinUI to either version 6.3.6 or 6.3.0-dev.6, but I'm getting the same error. Any suggestions?

14>D:\Projects\DMXCore\100\Software\Shared\Shared.csproj : error NU1605: Warning As Error: Detected package downgrade: Uno.Toolkit.Skia.WinUI from 6.3.6 to 6.3.0-dev.6. Reference the package directly from the project to select a different version. 
14>D:\Projects\DMXCore\100\Software\Shared\Shared.csproj : error NU1605:  DMXCore100 -> Uno.UI.HotDesign 1.0.160 -> Uno.Toolkit.Skia.WinUI (>= 6.3.6) 
14>D:\Projects\DMXCore\100\Software\Shared\Shared.csproj : error NU1605:  DMXCore100 -> Uno.Toolkit.Skia.WinUI (>= 6.3.0-dev.6)

@HakanL
Copy link

HakanL commented Dec 17, 2024

I was able to build now, after doing some rebuild all, etc. But not sure what caused the issue with the Toolkit, and not sure if it'll come back, but I'm good for now.

@jeromelaban
Copy link
Member

Thanks for the update@HakanL. I wonder, do you any animated controls, like a progressring or progressbar in your app, even if it's not visible?

@HakanL
Copy link

HakanL commented Dec 17, 2024

Thanks for the update@HakanL. I wonder, do you any animated controls, like a progressring or progressbar in your app, even if it's not visible?

I have a disabled ProgressBar on the splash screen, but I tried to remove it and it made no difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromelaban jeromelaban jeromelaban left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Raspberry Pi CM4 High CPU usage
4 participants
@ramezgerges @unodevops @HakanL @jeromelaban