Fix ARM Build for UWP Release #1391
Conversation
JimBobSquarePants
changed the title
|
Hey, this is awesome! 🎉🎉🎉 I mean, I'm sad to see that at the end of the day this build error was a consequence of the whole value delegate trick, but thankfully it still seems to work just fine in all other processors on UWP, and the issue was only when building for ARM32 anyway. It's great that the .NET Native team could identified exactly the faulting methods so we can just deoptimize them instead of burning performance for the whole library, yay! 🚀 |
Codecov Report
@@ Coverage Diff @@
## master #1391 +/- ##
=======================================
Coverage 82.83% 82.84%
=======================================
Files 690 690
Lines 30843 30848 +5
Branches 3542 3542
=======================================
+ Hits 25550 25555 +5
Misses 4572 4572
Partials 721 721
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| { | ||
| ref int histogramBase = ref MemoryMarshal.GetReference(this.histogramBuffer.GetSpan()); | ||
| ref TPixel pixelBase = ref MemoryMarshal.GetReference(this.source.GetPixelRowSpan(y)); | ||
| int levels = this.luminanceLevels; |
There was a problem hiding this comment.
I see some bonus loop hoisting, nice 🚀
| /// <inheritdoc/> | ||
| #if NETSTANDARD2_0 | ||
| // https://github.com/SixLabors/ImageSharp/issues/1204 | ||
| [MethodImpl(MethodImplOptions.NoOptimization)] |
There was a problem hiding this comment.
Shame that we can't just target UWP specifically for this, but .NET Standard 2.0 as a whole should mostly be for back-compat anyway, so this seems absolutely fine to me. This whole processor is not one of the most used though, so this shouldn't affect most code paths and usages of the library anyway, which is great.
There was a problem hiding this comment.
Ha! Just typed the same thing below 😝
|
@Sergio0694 Yeah, really glad to have made some progress here. Would have been nice to have been able to target a little more specifically but I don't believe it's possible (is it?) |
|
@JimBobSquarePants It actually is, though it requires some more setup. This might not be worth it though considering that both UWP as a whole (I mean, .NET Native) and .NET Standard 2.0 are basically mostly considered legacy at this point, as people gradually just move to .NET 5. Given that the change is so small to fix this, it might be worth it to just keep the library as is. Thoughts? |
|
Sounds like a lot of extra work/testing for little reward. Let's stick to what we have for now. |
|
The day UWP finally switches to .NET 6 I will pop the biggest bottle of champagne ever made 🤣 |
|
I'll drink to that! 🍾 |
Fix ARM Build for UWP Release
Prerequisites
Description
Fixes #1204
From conversations with the .NET Native team. Thanks @Sergio0694 for arranging that.
The compiler actually fails unless both
CdfApplicationRowOperation.InvokeandGrayscaleLevelsRowOperation.Invokeare marked withMethodImplOptions.NoOptimization.I attempted to work around the problem by manually inlining the luminance method call and better managing scope per loop iteration. While this didn't work they should result in better performance across all platforms.
I've also added notes for potential further operations.