-
-
Notifications
You must be signed in to change notification settings - Fork 853
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
Mono AOT decoder workaround for slow jpeg decoding. #2762
Mono AOT decoder workaround for slow jpeg decoding. #2762
Conversation
I don't think this PR had any impact for what I have tested so far (assuming I built the PR correctly), but I also didn't see the super slow loads the initial person reported. I do still need to test Android. I have a test repo here. It is a .NET 8 MAUI application. I will test Windows and Mac out of curiosity. I won't test Xamarin.Forms (Xamarin.iOS/Xamarin.Android) The readme has instructions on how I built this PR into a local nuget package, and how I tested the project on my iPhone 15 Pro Max in both debug and release mode. The test image I am using is located in ImageSharpMAUITest/ImageSharpMAUITest/Resources/Raw/sloth.jpg The ImageSharp code I am using is here. I am loading the image from a stream in the raw folder. I don't think there is any overhead from loading it like this instead of passing file on disk. using (var stream = await FileSystem.OpenAppPackageFileAsync("sloth.jpg"))
{
using (var image = await SixLabors.ImageSharp.Image.LoadAsync(stream))
{
...
}
} ResultsUpdating these results as they come in Debug (this PR)
Debug (3.1.4)
Release (this pr)
Release (3.1.4)
Test devices
|
Added all the numbers. My takeaway from this (as someone who uses images, but doesn't really know much about image encoding) is:
More than happy to add and run more tests as requested. Attaching the raw results numbers below (if anyone wants to see the results per run). |
@beeradmoore I've been doing some R&D in order to understand the Android performance. I can't see a configuration value in your sample to enable LLVM which is required for maximum performance. Did I miss something? https://learn.microsoft.com/en-us/dotnet/android/building-apps/build-properties#enablellvm |
I did not. I added
And during executing all the tests it gets to about 15/40 (half way through jpg load and resize run) and it crashes with
This is new to me and I have no idea what or why it is doing this. I have re-installed AndroidOS between then and now, this could be related, it could be AOT compile. Tonight I can revert back to not having LLVM and see if to happens again or not. It could also be I did no have developer mode enabled so maybe it was going to sleep in 15sec 🤷♂️ I did a single run of with v3.1.4 in release mode running the I referred to the previous thread and grabbed info from this comment, and just put
in my main With that I got 516ms for the local nuget. Huge improvement! I can test again tonight to see what ones of those above properties are required, if my release Android targeting below was working as intended. |
@beeradmoore Did you ever get those updated Android benchmarks? |
@JimBobSquarePants , aside from that single test I did not. Creating a reminder to do it this weekend. |
Thanks @beeradmoore very much appreciated! |
So some more checking into things. For these results I will call things run #1 - #6 with each run having a different csproj config. These different configs can also impact filesize. I think we are also focusing on changes to Android, so these tests are only on Release mode on my physical Pixel 2 XL. If there are any specific AOT flags or anything you want me to re-run on other platforms just let me know what ones. Settings used for each run: Run 1Of the 3 options listed in the other runs, this run did not have them in the csproj. I did not lookk what the net8.0-android defaults are for release mode. Run 2<EnableLLVM>true</EnableLLVM> This resulted in a crush mid way through the second test. While the tests could be ran individually there is an issue if you can't run them back to back so I am considering these invalid.
More info on these crashes below. Run 3<EnableLLVM>true</EnableLLVM>
<RunAOTCompilation>true</RunAOTCompilation> This also resulted in a crash.
More info on these crashes below. Run 4<EnableLLVM>true</EnableLLVM>
<RunAOTCompilation>true</RunAOTCompilation>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot> Run 5<EnableLLVM>true</EnableLLVM>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot> Same as Run 4, but without the Run 6<EnableLLVM>false</EnableLLVM>
<AndroidEnableProfiledAot>true</AndroidEnableProfiledAot> This is the same as run 5 but it swaps Results (3.1.4)
Results (this pr)
CrashesThe crashes looks like they are discussed in this issue in the dotnet runtime repo. ConclusionMy takeaway from this is the currently release 3.1.4 and this PR perform the same, at least for the limited tests I am doing. Other situations, or possibly other devices, may perform differently. What the new code is doing is above my skillset, but I don't see it degrading performance so I don't think its a bad change. If people want more performance with ImageSharp for Android then they will get it at an incrased filesize by enabling these options for release mode Android builds. <PropertyGroup Condition="$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)')) == 'android' AND '$(Configuration)' == 'Release'">
<EnableLLVM>true</EnableLLVM>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot>
</PropertyGroup> Using I am unsure if there are more optimisations you can do, or if the next step of improvemnets requires the dotnet team to do things on their end. I am also unsure if writing C# code using Android SDK for image manipulation gets it going any faster or if I am now approching limits on my 7 year old phone. Doing so would then require more devtime to write platform specific code for every single image manipulation piece I'd do for each platform I'd run my app on. It's also then just a lot more code in general to maintain. Whereas ImageSharp appears to do a great job across the board, except for Android where it just does an ok job OR I just use pngs everywhere, becaause they were always fast everywhere 😅 |
Thanks @beeradmoore for the monumental effort you've put in here. I think this should actually enable us to close off several longstanding issues. Judging by this comment and this comment the following combination should be the recommended one for Android. <EnableLLVM>true</EnableLLVM>
<RunAOTCompilation>true</RunAOTCompilation>
<AndroidEnableProfiledAot>false</AndroidEnableProfiledAot> iOS performance looks fantastic and Android, I imagine will improve with new phones and codegen improvements. I think most complaints are either outdated or the result of miscofiguration. Adding the Android and iOS specific MAUI configuration from your sample with an explanation to this section of the docs will help eleviate issues going forward. Despite relative equality of the performance metrics of the two source versions I think merging this PR is still a wise choice. It's much easier to maintain internally as it removes misdirection. |
I think one of the things that will catch people off guard is running in debug mode being slower. I would expect things to be slower in debug mode, but not 14sec to load an image. That would have caught me off guard and I would have questioned the library long before trying in release mode. |
Yeah... Definitely need to docuement that also and raise issues with the relevant parties. |
I've added configuration notes to our docs. Please let me know if I've missed anything. https://docs.sixlabors.com/articles/imagesharp/gettingstarted.html#maui-performance |
Prerequisites
Description
This PR is an attempt to work around issues found with the Mono AOT compiler which causes slow performance on IOS Android
dotnet/runtime#71210
In the linked Issue, Analysis from the Mono team highlighted this indirection as a culprit.
This PR removes that indirection completely by introducing an internal base class,
ImageDecoderCore
for allXXDecoderCore
instances. In addition, seeding has been introduced forSpectralConverter<TPixel>
and others.This is currently untested but I'm confident that this should improve matters.