Remove Custom Cache in ImageEx to evaluate binary impact size #3736

michael-hawker · 2021-02-05T19:38:07Z

Quick test to evaluate the footprint of this individual control on the package.

Based off PR #3727

Comparing against #3733, wondering if it's this single API or not that has an effect, as everything else seems rather common or using other things already being used in other APIs in the Toolkit...

Updated as we identified the custom caching in #2486 is the culprit to the application footprint. Reverting to just use System Cache with the applied fix still to evaluate final impact again with compiled dlls.

This should save around 675kb to optimized app footprint.

Also Fixes #3741 removing unused element.

ghost · 2021-02-05T19:38:10Z

Thanks michael-hawker 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 🙌

Too Large an Impact on App Binary Size 675kb

Fixes CommunityToolkit#3741

michael-hawker · 2021-02-08T02:52:13Z

Validated this fix does produce the validated savings in the final compiled binary. Just need to decide if we take it. Otherwise good to review.

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

Rosuavio

Looks good. If you want to openup custom cashing strategies to users in this PR or another, I think both work fine.

…sing ImageExBase/ImageEx Removed class level fields which were really local variables. Made new virtual methods. Still works fine in Sample App.

Can now use the sample button as a toggle. Close button is now visible. Made grid taller so sample is more apparent on high resolution monitors.

michael-hawker · 2021-02-11T19:18:34Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

+        /// </example>
+        /// <param name="imageUri"><see cref="Uri"/> of the image to load from the cache.</param>
+        /// <returns><see cref="Task"/></returns>
+        protected virtual Task ProvideCachedResourceAsync(Uri imageUri)


Should I rename this as AttachCachedResourceAsync? Or should I have it return the resource and call AttachSource in the parent method? Thoughts?

I am leaning towards the override method returns the resource. I think it might be simpler for the consumer and for the implementation.

Would it be something like

protected virtual ImageSource GetSource(Uri image) { return new BitmapImage(image); }

I guess the control would have to call AttachSource it's self. Would the control have to handle the lock? Would that be a good thing, rather then let the consumer deal with it?

Rosuavio

Looks good, I think the docs on ProvideCachedResourceAsync will be helpful.

Sergio0694

Looks nice, and love how these changes will keep the same flexibility but reduce the file size a lot! Just left a few minor observations and ideas, but overall it looks good 😄

Sergio0694 · 2021-02-16T18:10:16Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

+        /// <summary>
+        /// Gets value tracking the currently requested source Uri. This can be helpful to use when implementing <see cref="AttachCachedResourceAsync(Uri)"/> where loading an image from a cache takes longer and the current container has been recycled and is no longer valid since a new image has been set.
+        /// </summary>
+        protected Uri CurrentSourceUri { get; private set; }


I'm not 100% convinced about this property 🤔

As in, it seems to me that this is only mean to be used by inheriting classes overriding the loading method to check against the Uri they have as an argument, to detect whether the image has been changed while they were still loading. In that case wouldn't it be clearer and potentially less error prone to instead just keep a private field of type CancellationTokenSource, and pass a token to that virtual LoadCachedImageAsync method? That was people implementing that would just need to check whether or not the token is canceller, or they could just as easily just call [CancellationToken.ThrowIfCancellationRequested(https://docs.microsoft.com/en-us/dotnet/api/system.threading.cancellationtoken.throwifcancellationrequested?view=net-5.0), since the corresponding exception is already property handled by the callee anyway. The control would just need to cancel the token whenever the image is changed, and then assign a new token source if needed and pass the token downstream.

Thoughts?

Yup, sounds good.

Sergio0694 · 2021-02-16T18:12:08Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

+        /// Method to call to assign an <see cref="ImageSource"/> value to the underlying <see cref="Image"/> powering <see cref="ImageExBase"/>.
+        /// </summary>
+        /// <param name="source"><see cref="ImageSource"/> to assign to the image.</param>
+        protected void AttachSource(ImageSource source)


This method is called by one of the earlier conditional branches in the method SetSource below, but it's not resetting the local CurrentSourceUri property. Doesn't this mean that technically the image might load an image synchronously from there while at the same time a previous cached image loading is still asynchronously processing, which would end up overriding the final image once it completes? Since the CurrentSourceUri property would still be the same, so that overridden method wouldn't be able to detect a change in the control state 🤔

This is handled with the Token.

Sergio0694 · 2021-02-16T18:15:06Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

            }

-            await LoadImageAsync(_uri);
+            await LoadImageAsync(CurrentSourceUri);


This await call is done in an async void method, and the callee doesn't have a try/catch block - if there was an exception the whole process would likely crash. Is this intentional, or would we want to add a safety path there to instead just display nothing and raise the ImageFailed event and corresponding visual state?

I see that the various try/catch blocks are mentioned in the docs for the custom LoadCachedImageAsync instead, what if we used them here instead so that they'd automatically work for overridden methods too, but they'd also at the same time shield the control from other possible crashes in the default implementation?

Done, we handle everything now. I did find that the original code was calling open/failed twice, as that's handled automatically by us attaching the source, so that should be all cleaned-up now too in the new commit.

Sergio0694 · 2021-02-16T18:16:10Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs

+        /// catch (OperationCanceledException)
+        /// {
+        ///     // nothing to do as cancellation has been requested.
+        /// }
+        /// catch (Exception e)
+        /// {
+        ///     lock (LockObj)


Just a small note to remind to update this XML docs and remove the try/catch logic if we do decide to go with the previous idea and to move this control flow logic up one scope, since consumers here wouldn't need to repeat this anymore at that point. 🙂

…attern Automatically handle failure cases for image loading, added comment to clarify events. Do better type checks & cleaned-up calls to type conversions

ghost · 2021-02-17T00:21:54Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

ghost · 2021-02-17T20:04:04Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Sergio0694

Looks good!
Left another couple of nitpicks but I'm still approving this as I really don't want to block this just because I found two more minor details to possibly improve, they're nothing major anyway 😄

The refactoring itself and the new way the control works are great! 🚀

Sergio0694 · 2021-02-17T20:20:26Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs


-            this._tokenSource = new CancellationTokenSource();
+            _tokenSource = new CancellationTokenSource();


Nit: we could move the initialization of this instance down below after our synchronous paths are taken, so that in case the image is loaded synchronously instead we can skip the allocation here entirely. Basically we really only need to create this if ProvideCachedResourceAsync is called, otherwise we don't use the token anywhere else 🙂

Sergio0694 · 2021-02-17T20:23:59Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs


-            this._tokenSource = new CancellationTokenSource();
+            _tokenSource = new CancellationTokenSource();

            AttachSource(null);


Question: is toggling this visual state unconditionally by design? Because if the input is not null, doing this here no matter what means we're basically always cycling between loaded -> unloaded -> loading -> loaded whenever a new image is displayed.

I'm thinking maybe we could skip a couple of visual states here and:

Only switch to the unloaded state when we're actually setting a null source

Only set the loading state if we're loading an image asynchronously (that is, in the first two paths of our LoadImageAsync method.

It's a minor change but in theory it might help with performance a tiny bit in cases where we have a lot of controls being displayed eg. in a virtualized list, as those would be toggling the target image all the time?
Just an idea 😄

michael-hawker · 2021-02-17T20:56:01Z

Thanks @Sergio0694 I think this is a good first step, we can follow-on, add some tests, and optimize the states as a follow-on.

michael-hawker added the DO NOT MERGE ⚠️ label Feb 5, 2021

ghost requested review from azchohfi, Kyaa-dost and Rosuavio February 5, 2021 19:38

michael-hawker force-pushed the test-chop-imageex branch from e46b371 to 508a3bb Compare February 6, 2021 01:15

michael-hawker changed the title ~~DO NOT MERGE - Test remove Casting reference in ImageEx to evaluate binary impact size~~ DO NOT MERGE - Test remove Custom Cache in ImageEx to evaluate binary impact size Feb 6, 2021

Remove Custom Caching Strategy from ImageEx

e24db50

Too Large an Impact on App Binary Size 675kb

michael-hawker force-pushed the test-chop-imageex branch from 508a3bb to e24db50 Compare February 6, 2021 06:58

Remove old Unused ProgressRing from ImageEx control

c43f68a

Fixes CommunityToolkit#3741

michael-hawker force-pushed the test-chop-imageex branch from 95d819e to c43f68a Compare February 6, 2021 09:35

Merge branch 'dev/split-controls' into test-chop-imageex

0ad2fc9

michael-hawker marked this pull request as ready for review February 8, 2021 02:52

ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Feb 8, 2021

michael-hawker changed the title ~~DO NOT MERGE - Test remove Custom Cache in ImageEx to evaluate binary impact size~~ Remove Custom Cache in ImageEx to evaluate binary impact size Feb 8, 2021

michael-hawker commented Feb 8, 2021

View reviewed changes

Microsoft.Toolkit.Uwp.UI.Controls.Core/ImageEx/ImageExBase.Source.cs Outdated Show resolved Hide resolved

michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Feb 8, 2021

Rosuavio approved these changes Feb 8, 2021

View reviewed changes

Kyaa-dost linked an issue Feb 9, 2021 that may be closed by this pull request

VoiceCommand support using SpeechRecognizer #3392

Closed

Kyaa-dost removed a link to an issue Feb 9, 2021

VoiceCommand support using SpeechRecognizer #3392

Closed

Merge branch 'dev/split-controls' into test-chop-imageex

dbd379c

michael-hawker removed the DO NOT MERGE ⚠️ label Feb 10, 2021

michael-hawker marked this pull request as draft February 10, 2021 18:29

michael-hawker added 2 commits February 11, 2021 11:15

Update ImageEx to support custom implementation of Caching by subclas…

628d1d5

…sing ImageExBase/ImageEx Removed class level fields which were really local variables. Made new virtual methods. Still works fine in Sample App.

Fix issues with ImageEx Lazy Loading Sample

9c1dcdf

Can now use the sample button as a toggle. Close button is now visible. Made grid taller so sample is more apparent on high resolution monitors.

michael-hawker commented Feb 11, 2021

View reviewed changes

michael-hawker marked this pull request as ready for review February 11, 2021 19:19

michael-hawker requested a review from Rosuavio February 11, 2021 19:19

Rosuavio approved these changes Feb 11, 2021

View reviewed changes

Updated method name and provided more guidance in comments.

8239841

michael-hawker linked an issue Feb 12, 2021 that may be closed by this pull request

ProgressRing is referenced in ImageEx but not in Style #3741

Closed

michael-hawker changed the base branch from dev/split-controls to master February 12, 2021 21:41

Merge branch 'master' into test-chop-imageex

9d9f253

Sergio0694 requested changes Feb 16, 2021

View reviewed changes

ghost added the needs author feedback 📝 label Feb 16, 2021

Kyaa-dost and others added 2 commits February 16, 2021 14:53

Merge branch 'master' into test-chop-imageex

c2fc4ad

Apply PR feedback - Use CancellationToken instead, Simplify calling p…

b386021

…attern Automatically handle failure cases for image loading, added comment to clarify events. Do better type checks & cleaned-up calls to type conversions

ghost added needs attention 👋 and removed needs author feedback 📝 labels Feb 17, 2021

michael-hawker added auto merge ⚡ and removed needs attention 👋 labels Feb 17, 2021

Sergio0694 approved these changes Feb 17, 2021

View reviewed changes

Merge branch 'master' into test-chop-imageex

d29e7b9

ghost merged commit 189e91a into CommunityToolkit:master Feb 17, 2021

michael-hawker added this to the 7.0 milestone Feb 17, 2021

michael-hawker added the introduce breaking changes 💥 label Feb 17, 2021

This pull request was closed.


		this._tokenSource = new CancellationTokenSource();
		_tokenSource = new CancellationTokenSource();

Remove Custom Cache in ImageEx to evaluate binary impact size #3736

Remove Custom Cache in ImageEx to evaluate binary impact size #3736

Uh oh!

Conversation

michael-hawker commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 5, 2021

Uh oh!

michael-hawker commented Feb 8, 2021

Uh oh!

Uh oh!

Rosuavio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosuavio Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosuavio Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rosuavio left a comment

Choose a reason for hiding this comment

Uh oh!

Sergio0694 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 17, 2021

Uh oh!

ghost commented Feb 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Sergio0694 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-hawker commented Feb 17, 2021

Uh oh!

Uh oh!

michael-hawker commented Feb 5, 2021 •

edited

Loading

Rosuavio Feb 11, 2021 •

edited

Loading

Rosuavio Feb 11, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.