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

[Windows] Improve performance of ContentPanel.EnsureBorderPath #22346

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

Conversation

MartyIX
Copy link
Collaborator

@MartyIX MartyIX commented May 11, 2024

Description of Change

This PR is a simple performance improvement for ContentPanel on Windows.

The main change is in the 3rd commit (5056720), previous commits fix code style.

Performance comparison

Speedscope comparing main and this PR using testing code (main, PR):

image

How to repeat:

cd src/Controls/samples/Controls.Sample.Sandbox
dotnet publish -f net8.0-windows10.0.19041.0 -c Release -p:PublishReadyToRun=false -p:WindowsPackageType=None
dotnet trace collect --format speedscope -- .\bin\Release\net8.0-windows10.0.19041.0\win10-x64\publish\Maui.Controls.Sample.Sandbox.exe

and click Batch Add Borders button FIVE times.

Issues Fixed

Contributes to #21087


_borderPath.UpdatePath(_borderStroke?.Shape, width, height);
UpdateClip(_borderStroke?.Shape, width, height);
}

internal void EnsureBorderPath()
internal void EnsureBorderPath(bool containsCheck = true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also invoked from

handler.PlatformView.EnsureBorderPath();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. I believe one cannot use containsCheck = false there though so I did not do it.

If it is not what you meant, please clarify.

Copy link
Contributor

Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label May 11, 2024
@MartyIX MartyIX changed the title [Windows] ContentPanel performance [Windows] ContentPanel performance regarding EnsureBorderPath May 11, 2024
@MartyIX MartyIX changed the title [Windows] ContentPanel performance regarding EnsureBorderPath [Windows] Improve performance of ContentPanel.EnsureBorderPath May 11, 2024
@MartyIX MartyIX marked this pull request as ready for review May 11, 2024 20:23
@MartyIX MartyIX requested a review from a team as a code owner May 11, 2024 20:23
@MartyIX MartyIX added legacy-area-perf Startup / Runtime performance platform/windows 🪟 labels May 11, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

Could you update again the Speedscope comparison screenshot?

@MartyIX
Copy link
Collaborator Author

MartyIX commented May 13, 2024

Here is the original one:

329810830-a613d26d-2bb6-47f6-a920-67bbf78f27f8

Or do you mean a new one?


_borderPath.UpdatePath(_borderStroke?.Shape, width, height);
UpdateClip(_borderStroke?.Shape, width, height);
}

internal void EnsureBorderPath()
internal void EnsureBorderPath(bool containsCheck = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also invoked from

handler.PlatformView.EnsureBorderPath();

@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community ✨ Community Contribution legacy-area-perf Startup / Runtime performance platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants