V12: Fix ImageSharp3 performance issue

[nikolajlauridsen]

During load testing of V12, we noticed that after upgrading to ImageSharp3, sites would occasionally have huge spikes in response times.

Throwing a profiler on it and doing some digging it seems that this is caused by calling _requestAuthorizationUtilities.ComputeHMAC in the ImageSharpImageUrlGenerator, the reason for it as far as I can tell is because ImageSharp will use their own utility AsyncHelper.RunSync to then run the async method, this, in turn, creates a new Task using the TaskFactory however, this seems to be causing major hangs. Without knowing the intricate insides of TaskFactory I assume that this is caused by some form of thread pool starvation. Changing the code over to _requestAuthorizationUtilities.ComputeHMACAsync(uri, CommandHandling.Sanitize).GetAwaiter().GetResult(); seems to resolve the issue.

We can see this pretty clearly from the hotspots in the profiling:

With ImageSharp2:

Looks pretty normal, nothing too out of the ordinary, but if we switch to ImageSharp3:

We see a HUGE spike in time spent on RunSync a whopping 35,8% 🤯

Now after this patch

We're back to normal, and the spikes are gone when load testing.

Testing

• Ensure that Imagas still loads with custom parameters (Clean Starter Kit is good for testing this)
• Try and enable HMAC protection and ensure that it works 👍

[bergmania]

LGTM 💪

[ronaldbarendse]

Looking into the ImageSharp.Web source code, computing of the HMAC token doesn't actually require async in the default implementation on OnComputeHMACAsync... 😕

@JimBobSquarePants This might be interesting for you too, because the Tag Helper that adds HMAC tokens also uses the sync method. It already short-circuits when the secret is not set, similar to what's now added in Umbraco's ImageSharpImageUrlGenerator, so it at least doesn't negatively impact sites that are not using this feature... But if it is enabled, that will have similar performance issues and will really add up when the amount of image links increases on a page.

Since computing the HMAC token is mostly CPU-bound and fairly quick, it might be better to change OnComputeHMACAsync to OnComputeHMAC to remove the async overhead completely. Or otherwise improve the performance of AsyncHelper and add more short-circuiting to RequestAuthorizationUtilities, so it doesn't create new instances of Uri and QueryCollection or calls QueryHelpers.ParseQuery() or ToHttpContext() when the HMAC secret is not set anyway.

[JimBobSquarePants]

@ronaldbarendse Thanks for flagging that with me.

I don't know why I made OnComputeHMACAsync at all. Probably lack of sleep.

What's the expected release date for Umbraco v12?

I ask because I need to ship a v3.1 of ImageSharp to fix a critical issue with animated gif generation and the fix contains a breaking change. This would let me also ship a v3.1 of ImageSharp.Web where I could fix this issue properly by removing the async version.

Obviously I want Umbraco to be able to ship a working solution before launching v12!

I've got some time coming up over the next month to look at getting things shipped as my family are going away but it's still gonna take me a few weeks to get everything done on my own.

I don't think it would be possible to squeeze proper performance out of AsyncHelper. It's an abuse to use it in this way anyway.

[bergmania]

What's the expected release date for Umbraco v12?

May 29, so that pretty soon

With breaking changes we should have to wait to Umbraco 13 (december) or make another assembly that uses 3.1 so it becomes available to people to opt in to that.

[JimBobSquarePants]

May 29

It's June 9th just now according to my calendar.

This gif issue is really critical. You're gonna get complaints. Likely a lot of them.

[bergmania]

Lol sorry, I cant spell :)

June 29.

Yes, that's why I suggest we can make a solution in a new assembly that uses 3.1

[JimBobSquarePants]

I might be able to make the 29th. I'll try my best.

[bergmania]

You're a ⭐

We'll monitor NuGet :)

[JimBobSquarePants]

OK. I've opened a PR for the middleware. I'll get stuck into the remaining fixes in ImageSharp main over the next week.

[bergmania]

Hi @JimBobSquarePants .

Any update about whether it is realistic or not to have 3.1 released before we release Umbraco 12?

[JimBobSquarePants]

@bergmania I tried my very best but am struggling to get code reviewed.

https://github.com/SixLabors/ImageSharp/pulls

[bergmania]

All good, no rush..

We will figure out 😄

[ronaldbarendse]

@JimBobSquarePants We're currently one month away from releasing Umbraco 13 LTS and would really like to take a dependency on the latest ImageSharp.Web that contains the proper fix for this performance issue (PR #327). This would allow us to remove the RequestAuthorizationUtilities.ComputeHMACAsync(...).GetAwaiter().GetResult() workaround with a simple RequestAuthorizationUtilities.ComputeHMAC(...) call and avoid the async overhead/synchronization context switching, which should improve performance ever so slightly.

This is just a 'nice to have' for the 13.0.0 release, as we can always bump the minimum version and fix the code, but I just wanted to check with you if you're planning to release 3.1 soon or if there's other things blocking it (and if so, if we're able to help).

[JimBobSquarePants]

Sorry for the slow reply @ronaldbarendse It's been a mental time for me lately.

I've had to put my head down completely rewriting the gif encoder in ImageSharp plus fixing some significant issues, so the release has come much slower than planned.

I can see you've been really helpful in the web discussion repo so thanks for that. I'd really love your insight here also if you can? SixLabors/ImageSharp.Web#339

I've only got a few things to sort now upstream and am planning on a release hopefully by 30th Nov. I'll have a big PR that I'll need someone to help review that fixes and normalizes all animated encoders so if you could help review that I'd be very much obliged. I'll ping you when it's ready.

[JimBobSquarePants]

Tada!

https://www.nuget.org/packages/SixLabors.ImageSharp.Web/3.1.0