Improve the ImageEx control performance

[h82258652]

Fixes #3250

Remove AsTask().ConfigureAwait(false) from two lines in CacheBase.cs.
I don't know why but it really works.

PR Type

What kind of change does this PR introduce?

• Bugfix

What is the current behavior?

If you load lots of images at the same time, it will block the UI very long.

What is the new behavior?

Load faster than before, it only blocks the UI 1/4 times long on my computer.

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• If new feature, add to Feature List
• Contains NO breaking changes

Other information

[ghost]

Thanks h82258652 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[vgromfeld]

@h82258652 The ConfigureAwait(false) should remain as much as possible. It usually provides better performances by not requiring a dispatch to the caller thread context.
This page contains a lot of details on how it works and when it should be used https://devblogs.microsoft.com/dotnet/configureawait-faq/.

My guess is that you have all the loading being run synchronously on the UI thread.
A quick test with the community toolkit sample app seems to confirm this behavior.

If you are loading a lot of images, most of the code from ImageEx ends being executed synchronously. Most the async tasks started by the control are in the completed state before being awaited. Since ConfigureAwait(false) is used, all the continuations are run on the UI thread and you end with everything being done in the UI thread.

I haven't tried it yet but looking at the code it seems that add a Task.Run() in LoadImageAsync() (or somewhere else) would solve the issue by forcing all the image processing to be done in the thread pool.

[h82258652]

@vgromfeld You are right. ConfigureAwait(false) means don't capture the caller thread context.
I think the problem is AsTask, it is not open source, no one knows what it does.

I try to add Task.Run to LoadImageAsync() and get an exception, there are some codes dependent by the UI thread. And I also try to add Task.Run to GetFromCacheOrDownloadAsync in CacheBase.cs, it still blocks the UI very long.

[vgromfeld]

I tried to investigate more this week-end but still didn't found the root cause. I found that CacheBase is calling ImageCache.InitializeTypeAsync() which is deferring some work to the UI thread to create and load the BitmapImage. I'm still unsure if it is the bottleneck or not.

[michael-hawker]

@Sergio0694 curious on if you have any thoughts in this area? I'm hesitant to do anything in this area without us knowing the root cause and understanding a proper fix.

@vgromfeld @azchohfi I'm thinking we may want to just defer this to 7.0 and maybe investigate a revamp or audit of the ImageEx control?

[azchohfi]

The ConfigureAwait(false) should speed things up, not slow them down... I wonder if this has anything to do with the AsTask(), but I honestly don't know how it works internally. I believe we should defer this to 7.0.

[vgromfeld]

Agree to move this to 7.0. My current guess is a thread pool issue with too many tasks competing for the UI thread but this will require more tests to validate.

[Sergio0694]

Sorry for the delat @michael-hawker - completely missed the notification here! 😅

I agree with @azchohfi on this, the culprit is very unlikely to be the ConfigureAwait(false) call since that doesn't really add overhead. It's more likely that that AsTask is causing the slowdown for some reason. One easy way to test this theory would be for @h82258652 to just run his app leaving either just AsTask(), or with AsTask().ConfigureAwait(true).

To elaborate on ConfigureAwait, all it's doing is hijacking the task awaiter used by default by tasks (TaskAwaiter<TResult>), with a ConfiguredTaskAwaitable<TResult>.ConfiguredTaskAwaiter instance. Both types are essentially the same (both are structs, so they don't allocate, etc.), and really the only difference is that the latter passes a custom value to the continueOnCapturedContext parameter for the internal continuation methods, whereas the former always passes true (as in, it always captures the original context). So if eg. you tried with AsTask().ConfigureAwait(true) you'd essentially get the same exact behavior as not invoking that method at all. If that still gives you the same slowdown, then yeah I'd say the culprit here would be that AsTask call 🤔

[azchohfi]

@h82258652 did you have some time to look at this? Thanks!

[h82258652]

Sorry so late for the response.
@Sergio0694 @azchohfi
I change these two lines to AsTask().ConfigureAwait(true) and it doesn't block the UI again!

The effect equals to remove AsTask().
It only blocks the UI while AsTask().ConfigureAwait(false), that's so strange. 🤔

[Sergio0694]

@h82258652 Ok uh... This is indeed very strange 🤔
Well one thing we can exclude at least is that AsTask causing issues, since that apparently is unrelated.
To recap, from what I understand:

✅ await FooAsync() - OK
✅ await FooAsync().AsTask().ConfigureAwait(true) - OK
⛔ await FooAsync().AsTask().ConfigureAwait(false) - SLOW

The weird part is that in this case both changes should be unrelated from "the UI thread".
What I mean is that right above the line you edited you have this:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/83856cf8c7394e320d4d1b43730682318b6563b9/Microsoft.Toolkit.Uwp.UI/Cache/CacheBase.cs#L392

Which means that in theory the rest of the asynchronous method will already be in a thread pool thread and not in the original synchronization context, regardless of the changes done in this PR in particular. Also I'm really not sure why would this line in particular change anything, considering there are 4+ other ConfigureAwait(false) calls here.

At this point if I had to make a wild guess I'd say maybe that AsTask() is returning some custom WinRT-specific wrapper inheriting from Task which doesn't play well with ConfigureAwait(false) in this specific scenarios, but I wouldn't really know how to explain that - you'd probably need to ask some MS engineer working in that specific area in UWP to elaborate on that.

At this point @azchohfi I'd say it's a matter of deciding whether to just accept the PR as a temporary workaround or hold until we can actually understand what's going on here? If you're concerned about the flow to the UI thread, that ConfigureAwait call I linked should already guarantee that we're not there anymore, so at worst the overhead here would be in a thread pool anyway, so without noticeable hiccups on the UI side. If we wanted to double check this just to be 100% sure, @h82258652 would you mind checking the HasThreadAccess on the UI thread right after the line I linked? That is, right before the line you edited. You can do that by eg. grabbing the dispatcher from the current window and checking that property, or by storing a dispatcher queue for the UI thread before that call and checking the thread access from that after the previous await completes.

[h82258652]

@Sergio0694

Before the line you linked, the HasThreadAccess is true.

[vgromfeld]

Here is link that may help to investigate this issue: .NET Threadpool starvation, and how queuing makes it worse.

We may face the same kind of issue when trying to load a lot of images. The queueing between .Net Task and WinRT IAsyncOperation/IAsyncAction can be different and may trigger this starvation...

Using the Thread Pool ETW events and Diagnosing .NET Core ThreadPool Starvation with PerfView (Why my service is not saturating all cores or seems to stall), we should be able to confirm if it is the case or not.

[michael-hawker]

Going to convert this to a draft PR as we're still trying to figure out the best solution to this problem, thanks!

[michael-hawker]

@h82258652 have you looked through the guidance that both @Sergio0694 and @vgromfeld have provided? Curious if you're still planning to investigate a better solution or if we should close this PR for now?

[h82258652]

@michael-hawker
I have looked @Sergio0694 said and I agree with his opinion. It is very strange that should not happen if use .net framework or .net core. I think there is something wrong in the UWP runtime. I try to have a look at the AsTask source code here and here, but nothing found, at least I can make sure the code has no problems.
For @vgromfeld guide, I'm very sorry, I'm not good at the Windows Event Viewer (normally I use logs in my development). There are lots of events and I feel hard to filter the things I want.
It seems we can't make much progress in the short term and I'm moving my energy to my image control library these months. I think you can close this PR for now and just keep the issue is open.

[michael-hawker]

Thanks @h82258652 I'll close this for now and point in the issue to the discussion in the PR here.