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] Cache full app package file path #21246

Merged

Conversation

MartyIX
Copy link
Contributor

@MartyIX MartyIX commented Mar 16, 2024

Description of Change

I have noticed that one can cache Package.Current.InstalledLocation.Path to save about 4 ms (measured in Debug mode) for each call of FileSystemUtils.PlatformGetFullAppPackageFilePath1 because there is no interop call to WinRT2.

Testing

/~https://github.com/MartyIX/maui/tree/feature/2024-03-16-app-path-optimization-testing (MartyIX@ab01bf4)

Issues Fixed

Contributes to #8594

Footnotes

  1. Called for each registered font (see Font loading impacts Windows desktop startup #8594).

  2. Please correct me if I got it wrong.

@MartyIX MartyIX requested a review from a team as a code owner March 16, 2024 12:51
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 Mar 16, 2024
@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 16, 2024

cc @jonathanpeppers

@jsuarezruiz jsuarezruiz added platform/windows 🪟 legacy-area-perf Startup / Runtime performance labels Mar 18, 2024
@jonathanpeppers
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

I think this change makes sense.

In the attached issue, there was:

image

@MartyIX do you have a more recent .speedscope from your app? We should get a second data point on how much time this can save in a real application.

@jonathanpeppers
Copy link
Member

Oh, I found the .speedscope @MartyIX sent me last week:

image

Seems like an easy-win. 👍

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 18, 2024

I don't really know how to profile my real application with modified MAUI source code. So I'm testing with the sandbox project in MAUI source code.

However, I tested with MartyIX@ab01bf4 and it saved about 20 ms AFAIK. But it depends on the number of files. So if there is an app with 50 font files, then I would expect to save about 50 * <each call cost> and that can be certainly >100ms which is significant.

@jonathanpeppers
Copy link
Member

If you ever need to test a local MAUI build, I do:

  • dotnet cake --configuration=Release
  • wipe NuGet cache: rm -r ~\.nuget\packages\*\*-dev\ (if you ever do this more than once)
  • then build run an app with MAUI's .\bin\dotnet\dotnet.exe instead of the system-installed dotnet.exe

This might be trickier with a Windows app that is a "packaged" appx, though. Might have to launch VS with %PATH% set to .\bin\dotnet\.

@MartyIX
Copy link
Contributor Author

MartyIX commented Mar 26, 2024

@rmarinho Is this OK to merge? Or are there any concerns?

@rmarinho rmarinho requested review from mattleibow and removed request for StephaneDelcroix and tj-devel709 March 26, 2024 11:23
@rmarinho
Copy link
Member

pinging @mattleibow as he knows more about this that me :)

@mattleibow mattleibow merged commit 2d1bf58 into dotnet:main Mar 26, 2024
47 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 2024
@Eilon Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community ✨ Community Contribution fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457 legacy-area-perf Startup / Runtime performance platform/windows 🪟 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants