-
Notifications
You must be signed in to change notification settings - Fork 272
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
Add ImageSharp benchmark #2149
Add ImageSharp benchmark #2149
Conversation
Will have a look ASAP. A little concerned about how to maintain this. Ideally I would say that there would be a simple set of benchmarks that use a reference to ImageSharp via NuGet. That way anyone can PR to update the references when required. |
if you go ahead with this, there probably should be an entry in THIRD_PARTY_NOTICES.TXT I agree with @JimBobSquarePants isn't a NuGet reference to a fixed package version sufficient? There is a lot of code here that requires building, etc. |
That's what I have currently. It reference to the ImageSharp version 1.0.2 via nuget. The reason there are so many files are because I copied every single benchmark file present in the original repo modulo the other baseline benchmarks. Perhaps, if there is a subset of benchmarks that you think can represent a good coverage of ImageSharp, feel free to point out and I am happy to trim it to that subset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main challenge here is that these benchmarks were developed with local execution / sandboxing in mind, many of them are garbage for dotnet/runtime performance tracking. Tried to filter those out in my review.
I'm quite sure that most of the test images are unused by the benchmarks, those are better to be deleted.
With aggressive filtering this approach (of starting with the original IS benchmark project) can work out, but there is still a lot to delete. I wonder if it would save time, if you instead tried the other way: start with an (almost) empty project (containing only the TestEnfironment
infra bits), and include the benchmarks which bring value one-by-one. Less can be more here in the end of the day.
What I would include:
- Codecs, but only individual encode/decode benchmarks, not the ones that work on multiple files
- Individual processors, stuff like resize, rotate, blur etc.. (I couldn't find Resize, you will have to bring that here, that's our most optimized operation relying heavily on SIMD)
- Some of the other stuff, eg. color conversion
If you manage to get rid of the noisy stuff, I can do another review.
src/benchmarks/real-world/ImageSharp/Codecs/ImageBenchmarkTests.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/Jpeg/DecodeJpeg_Aggregate.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/EncodeBmpMultiple.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/EncodeGifMultiple.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/EncodeIndexedPng.cs
Outdated
Show resolved
Hide resolved
Done. |
@antonfirsov - Thanks for taking time and providing feedback. I have deleted all the benchmarks that you have suggested. Do you mind doing another round of review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pull in the new Jpeg benchmarks, add Resize benchmark, fix the memoryStream
issues this should be mostly good to go.
Any insights whether the variance of execution times is within acceptable range for these benchmarks?
src/benchmarks/real-world/ImageSharp/Codecs/DecodeFilteredPng.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/EncodeIndexedPng.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/real-world/ImageSharp/Codecs/Jpeg/DecodeJpeg_ImageSpecific.cs
Outdated
Show resolved
Hide resolved
Regarding the resize benchmark: We don't need the The subclasses exercising different scenarios are valuable, except |
@kunalspathak are you still planning to continue with this? |
Yes. I have been busy with Arm64 investigation work. Will get to this soon. I don't think much work is needed here. |
Ping to myself. |
@antonfirsov , @JimBobSquarePants - I will resume working on this project now. Wondering what should be the next steps? Should I update the ImageSharp code to more latest or should we get this in as-is (after addressing the review comments) and then do another PR to update the code base to the latest? |
If you are going to continue the snapshot approach I would recommend updating to the latest V2.x version before continuing. V1 numbers won't be beneficial and I can imagine you don't want to go through the PR process multiple times. However, for ease of maintenance I would suggest that the benchmarks are performed using NuGet packages. |
I agree. I will update the version in https://github.com/dotnet/performance/pull/2149/files#diff-99843982e8a5e1acef45cd069c3e3230ec1a014e7fe01aea4c05f2339b40afbc.
As part of this PR, ImageSharp will be consumed via nuget packages, if that's what you meant by "benchmarks are performed using NuGet packages.". My question was more around if I should take the latest benchmarks source code from ImageSharp repo or not. |
Done.
Done.
Done.
Could you confirm the exact version that I will put in |
2.1.3 |
I tried that and getting these errors:
Also I am getting these errors for this.Configuration.WorkingBufferSizeHintInBytes = this.WorkingBufferSizeHintInKilobytes * 1024;
|
@antonfirsov, @JimBobSquarePants - This is ready for the review. Let me know how I can upgrade to the latest ImageSharp nuget version. |
Apologies, I’ll review over the weekend |
I can understand things can be busy so just sending a friendly remainder in case it was forgotten. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error NU1102 Unable to find package SixLabors.ImageSharp with version (>= 2.1.3)
I don't see the cause of this issue, I would have to clone the repository and try it myself to figure out. Any chance you can try to investigate? I will be on vacation the rest of the week.
/// Is it worth to set a larger working buffer limit for resize? | ||
/// Conclusion: It doesn't really have an effect. | ||
/// </summary> | ||
public class Resize_Bicubic_Rgba32_CompareWorkBufferSizes : Resize_Bicubic_Rgba32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This benchmark is the cause of CS0281, it's not needed for non-regression purposes.
=> ctx.Resize(this.DestSize, this.DestSize, KnownResamplers.Bicubic, true); | ||
} | ||
|
||
public class Resize_Bicubic_Compare_Rgba32_Rgb24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, if a benchmark is about comparing A vs B, it's safe to trim it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kunalspathak this should be trimmed away,
I think I got this working with just 1 problem remaining. For some reason, I get compilation error while trying to invoke I tried clearing my nuget packages, but it doesn't help. |
The code you took is probably from the time before we changed - using Image img = this.decoder.Decode(Configuration.Default, this.preloadedImageStream);
+ using Image img = this.decoder.Decode(Configuration.Default, this.preloadedImageStream, default); |
that works. Wanted to make sure if |
Ping @antonfirsov and @JimBobSquarePants |
I cannot see anything out of the ordinary 👍 |
Thanks for confirming @JimBobSquarePants and thank you for the valuable feedback. @sblom - This one is another "real-world" benchmark we would like to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good in general, but noticed one point from previous review that was not addressed. Might worth a final self-review if there's anything left that can be trimmed away.
=> ctx.Resize(this.DestSize, this.DestSize, KnownResamplers.Bicubic, true); | ||
} | ||
|
||
public class Resize_Bicubic_Compare_Rgba32_Rgb24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kunalspathak this should be trimmed away,
We are now with following benchmarks:
|
This is a snapshot taken from SixLabors/ImageSharp@413c90c. I deleted
System.Drawing*
related benchmarks and most of the other 3rd party baseline benchmarks to keep things simple. I also copied fewTestUtilities
related code and the input images.I ran it on Windows/x64 and Windows/arm64 and they seem to be running fine. It needs some more refining to get 100% pass rate.
Relevant discussion: SixLabors/ImageSharp#1795 (reply in thread)
FYI - @antonfirsov , @JimBobSquarePants