wpf icon indicating copy to clipboard operation
wpf copied to clipboard

Published 20 hours ago • verified publisher icon dotnet

Improve performance of DynamicResource usages

Open batzen opened this issue 3 years ago • 2 comments

Fixes Issue #4468

Description

The currently used implementation to keep track of DynamicResource usages is suboptimal as the list can grow quite large and scanning gets very expensive. That's caused by two issues:

  • Every call to FetchResource adds a new entry to the list
  • The list implementation is slow as it does not use keys

The improved implementation only adds items with the same key once by checking for existing items before adding new ones. As the new implementation uses keys, retrieval of items is very fast.

Customer Impact

Slow performance.

Testing

Tested the app from #4468

batzen avatar Oct 30 '21 18:10 batzen

@dotnet/wpf-developers any news on this one?

batzen avatar Mar 12 '22 19:03 batzen

Have you thought about how it can be tested? How did you confirm the performance is improved?

I asked about how we are expected to write tests in multiple of my PRs, but never got an answer...

Performance improvements were checked by testing the app from #4468.

batzen avatar Dec 15 '22 15:12 batzen

@batzen @miloush @kronic - We've got the results from our test pass and it looks like the PR can be taken. Currently, the only discussion whether or not the internal class deserves to be sealed or not is open (which can be addressed even later).

With that said, if there are no other concerns, I am going ahead and merging this PR. Let me know if you have objections.

pchaurasia14 avatar Jun 24 '24 15:06 pchaurasia14

No objections. I don't mind sealed either way - as discussed, it has a potential for perf improvements, although possibly not in this usage.

miloush avatar Jun 24 '24 16:06 miloush

Thanks @batzen ! Sorry, this took long to get merged.

pchaurasia14 avatar Jun 25 '24 09:06 pchaurasia14

@batzen Contratulations, you finally got it! 😉👍🏼🍾

lightwalker01 avatar Jun 25 '24 10:06 lightwalker01

In which release can we expect this fix?

znakeeye avatar Jun 25 '24 10:06 znakeeye

@znakeeye I think it will enter .NET 9.

lindexi avatar Jun 25 '24 12:06 lindexi

@batzen, we have encountered a regression due to these changes. As it turns out, we are not expecting a null resourceKey here, but we are validating the same check here. This results in an ArgumentNullException. Is expecting a non-null value as resourceKey thought of? and if so, the validation checks no longer seems to be valid.

Please find the sample repro here. Try running the Wpf.Ui.Gallery application and it should throw the above mentioned exception. It is excepted to fail with .NET9, preview 7 binaries.

harshit7962 avatar Jul 08 '24 11:07 harshit7962

@harshit7962 Will have a look as soon as i can.

batzen avatar Jul 08 '24 15:07 batzen

Created PR #9393 to fix the issues

batzen avatar Jul 13 '24 06:07 batzen

@batzen, thank for your fix earlier, but even with the fix we are yet to close a few regression issues that we got to know of recently. I have created a sample repro here. The application is expected to crash with preview 7 (or rc1) binaries of .NET 9.

Apologies for this iterative reporting, hopefully we will be closing these in a single go this time.

harshit7962 avatar Jul 29 '24 06:07 harshit7962

@harshit7962 The first error i encounter is an exception because a SolidColorBrush is being used as a Color inside of ButtonStyles.xaml in line 2 where it says <SolidColorBrush x:Key="Menu.Static.Background" Color="{DynamicResource SolidBackgroundFillColorSecondaryBrush}" />.

This was also an error before my changes, but the exception was thrown at a different point in time. As the line was changed in Text-Grab last year and no one seems to have noticed that the background stays transparent because the resource has the wrong type, IMHO the new behavior is much better as the error would have been very obvious.

Before my changes the exception was thrown during rendering, caught there and the program just continued:

Unhandled exception: System.InvalidOperationException: '#FF1C1C1C' is not a valid value for property 'Color'.
   at System.Windows.DependencyObject.GetEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, RequestFlags requests)
   at System.Windows.DependencyObject.GetValueEntry(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, RequestFlags requests)
   at System.Windows.DependencyObject.GetValue(DependencyProperty dp)
   at System.Windows.Media.SolidColorBrush.get_Color()
   at System.Windows.Media.SolidColorBrush.UpdateResource(Channel channel, Boolean skipOnChannelCheck)
   at System.Windows.Media.SolidColorBrush.AddRefOnChannelCore(Channel channel)
   at System.Windows.Media.Brush.System.Windows.Media.Composition.DUCE.IResource.AddRefOnChannel(Channel channel)
   at System.Windows.Media.RenderData.System.Windows.Media.Composition.DUCE.IResource.AddRefOnChannel(Channel channel)
   at System.Windows.UIElement.RenderContent(RenderContext ctx, Boolean isOnChannel)
   at System.Windows.Media.Visual.UpdateContent(RenderContext ctx, VisualProxyFlags flags, Boolean isOnChannel)
   at System.Windows.Media.Visual.RenderRecursive(RenderContext ctx)
   at System.Windows.Media.Visual.UpdateChildren(RenderContext ctx, ResourceHandle handle)
   at System.Windows.Media.Visual.RenderRecursive(RenderContext ctx)
   at System.Windows.Media.Visual.UpdateChildren(RenderContext ctx, ResourceHandle handle)
   at System.Windows.Media.Visual.RenderRecursive(RenderContext ctx)
   at System.Windows.Media.Visual.Render(RenderContext ctx, UInt32 childIndex)
   at System.Windows.Media.CompositionTarget.Compile(Channel channel)
   at System.Windows.Media.CompositionTarget.System.Windows.Media.ICompositionTarget.Render(Boolean inResize, Channel channel)
   at System.Windows.Media.MediaContext.Render(ICompositionTarget resizedCompositionTarget)
   at System.Windows.Media.MediaContext.RenderMessageHandlerCore(Object resizedCompositionTarget)
   at System.Windows.Media.MediaContext.RenderMessageHandler(Object resizedCompositionTarget)
   at System.Windows.Media.MediaContext.Resize(ICompositionTarget resizedCompositionTarget)
   at System.Windows.Interop.HwndTarget.OnResize()
   at System.Windows.Interop.HwndTarget.HandleMessage(WindowMessage msg, IntPtr wparam, IntPtr lparam)
   at System.Windows.Interop.HwndSource.HwndTargetFilterMessage(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled)
   at MS.Win32.HwndWrapper.WndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled)
   at MS.Win32.HwndSubclass.DispatcherCallbackOperation(Object o)
   at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs)
   at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler)

After my changes the exception is thrown during BAML processing:

Unhandled exception. System.Windows.Markup.XamlParseException: 'Set property 'System.Windows.ResourceDictionary.Source' threw an exception.' Line number '17' and line position '18'.
 ---> System.InvalidOperationException: '#FF1C1C1C' is not a valid value for property 'Color'.
   at System.Windows.DependencyObject.EvaluateExpression(EntryIndex entryIndex, DependencyProperty dp, Expression expr, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry newEntry) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 1809
   at System.Windows.DependencyObject.EvaluateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry newEntry, OperationType operationType) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 1939
   at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 1348
   at System.Windows.DependencyObject.InvalidateProperty(DependencyProperty dp, Boolean preserveCurrentValue) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 1210
   at System.Windows.DependencyObject.InvalidateProperty(DependencyProperty dp) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 1179
   at System.Windows.ResourceReferenceExpression.InvalidateTargetProperty(Object sender, EventArgs e) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\ResourceReferenceExpression.cs:line 388
   at System.Windows.ResourceReferenceExpression.InvalidateExpressionValue(Object sender, EventArgs e) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\ResourceReferenceExpression.cs:line 383
   at System.Windows.DependencyObject.OnInheritanceContextChanged(EventArgs args) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 2649
   at System.Windows.Freezable.AddInheritanceContext(DependencyObject context, DependencyProperty property) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\Freezable.cs:line 1180
   at System.Windows.DependencyObject.ProvideSelfAsInheritanceContext(DependencyObject doValue, DependencyProperty dp) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 877
   at System.Windows.DependencyObject.ProvideSelfAsInheritanceContext(Object value, DependencyProperty dp) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\WindowsBase\System\Windows\DependencyObject.cs:line 850
   at System.Windows.ResourceDictionary.AddInheritanceContext(DependencyObject inheritanceContext, Object value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\ResourceDictionary.cs:line 2306
   at System.Windows.ResourceDictionary.AddInheritanceContextToValues() in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\ResourceDictionary.cs:line 2335
   at System.Windows.ResourceDictionary.set_Source(Uri value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\ResourceDictionary.cs:line 261
   at System.Windows.Baml2006.WpfSharedBamlSchemaContext.<>c.<Create_BamlProperty_ResourceDictionary_Source>b__344_0(Object target, Object value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\Baml2006\WpfGeneratedKnownProperties.cs:line 7535
   at System.Windows.Baml2006.WpfKnownMemberInvoker.SetValue(Object instance, Object value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\Baml2006\WpfKnownMemberInvoker.cs:line 44
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(XamlMember member, Object obj, Object value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\System.Xaml\System\Xaml\Runtime\ClrObjectRuntime.cs:line 298
   at MS.Internal.Xaml.Runtime.ClrObjectRuntime.SetValue(Object inst, XamlMember property, Object value) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\System.Xaml\System\Xaml\Runtime\ClrObjectRuntime.cs:line 284
   --- End of inner exception stack trace ---
   at System.Windows.Markup.XamlReader.RewrapException(Exception e, IXamlLineInfo lineInfo, Uri baseUri) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\XamlReader.cs:line 509
   at System.Windows.Markup.WpfXamlLoader.Load(XamlReader xamlReader, IXamlObjectWriterFactory writerFactory, Boolean skipJournaledProperties, Object rootObject, XamlObjectWriterSettings settings, Uri baseUri) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\WpfXamlLoader.cs:line 175
   at System.Windows.Markup.WpfXamlLoader.LoadBaml(XamlReader xamlReader, Boolean skipJournaledProperties, Object rootObject, XamlAccessLevel accessLevel, Uri baseUri) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\WpfXamlLoader.cs:line 43
   at System.Windows.Markup.XamlReader.LoadBaml(Stream stream, ParserContext parserContext, Object parent, Boolean closeStream) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Markup\XamlReader.cs:line 1095
   at System.Windows.Application.LoadComponent(Object component, Uri resourceLocator) in C:\DEV\OSS_Own\dotnet\wpf_my_prs\src\Microsoft.DotNet.Wpf\src\PresentationFramework\System\Windows\Application.cs:line 468
   at Text_Grab.App.InitializeComponent() in C:\DEV\issues\wpf\5610\Text-Grab\Text-Grab\App.xaml:line 1
   at Text_Grab.App.Main()

batzen avatar Jul 29 '24 08:07 batzen

@batzen Agreed that the exception being thrown is the correct way, however this creates a compat problem for applications that currently use DynamicResource and rely on the default behaviour (not crashing) even when incorrect value is provided. With this fix currently all those apps would not be able to update to .NET 9 without XAML changes on their end. I think we should find a way to ensure backward compatibility in this case.

harshit7962 avatar Jul 29 '24 11:07 harshit7962

Can someone clarify what the migration expectations to newer versions of WPF are in general? Why are we expecting .NET Core 3 apps to run on .NET 9 without any change? Apps that cannot update to .NET 9 without recompilation can stay on .NET 8, that's why we have runtime config to specify which runtime should be used.

miloush avatar Jul 29 '24 14:07 miloush

I agree with @miloush here as I've been asking myself the same question.

This issue is also the prime example, essentially having a bug in your code that didn't only cause a crash thanks to the exception being swallowed. It just makes no sense in my opinion. And there's a history of such changes happening from version to version outside WPF, don't see why it should be any different when it's a step in the right direction.

h3xds1nz avatar Jul 29 '24 19:07 h3xds1nz

@batzen, we are trying to evaluate the regression radius of the changes at the moment. I have created a minimal repro of the above application here. The following is my findings as of now.

The crash is reported only when we include the following in our csproj.

<ItemGroup>
    <Resource Include="ButtonStyles.xaml">
        <Generator>MSBuild:Compile</Generator>
        <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Resource>
</ItemGroup>

Need your help in understanding this change in behavior.

Tagging @miloush and @h3xds1nz for their feedbacks and opinions.

harshit7962 avatar Jul 30 '24 18:07 harshit7962

Is this only XAML parsing issue or runtime too? (Since it's a dynamic resource, a wrong type can be provided at runtime. If that now crashes and didn't used to, it would be even more concerning than the XAML parsing which is apparent immediately)

miloush avatar Jul 30 '24 18:07 miloush

  • I've tested the same stupid thing against .NET 8 and it will crash in runtime (when you set a resource ref) the same way it will crash with this PR in latest master, same spot, same call-stack; when running EvaluateExpression with InvalidPropertyValue
  • So the issue is indeed only when parsing BAML/XAML; and imho now it crashes in the correct spot and it is the correct behavior. Even IntelliSense is gonna be angry at you once you execute it.

What I fail to understand in detail because I can't debug the previous version today is how did this change happened but I can imagine that the value was already DeferredReference at that point and that's why it didn't crash while evaluating.

h3xds1nz avatar Jul 30 '24 19:07 h3xds1nz

I agree that these are errors in the applications and not WPF, and I fully support the early, compile-time failure. However, already during one preview release there is evidence this change breaks things - and that doesn't happen very often, it is the reason why preview releases exist in the first place. It is also very late in the release cycle and I doubt any further changes to the codebase will be entertained for 9 without giving people an opportunity to test them.

I wonder whether we could have it failing compilation forward but not crash applications if they run on a newer runtime than they have been compiled with?

miloush avatar Jul 30 '24 20:07 miloush

Control the behaviour with a feature switch.

RussKie avatar Jul 30 '24 23:07 RussKie

Switch is my least favorite option, we keep piling (often unnecessary) switches and never remove them. I am also not quite sure how switch would achieve a feature to be available at compile time but not at runtime (and there might be not enough time to test the switch before RTM). I believe the agreement is we do want the changes in this PR to be part of the product and if the intent is to update the PR to avoid breaking these apps without a switch, then the PR can be updated and merged back later, unless there is an urgency that I missed.

(On the other hand you could say setting a .NET runtime version in the config is a switch.)

miloush avatar Jul 30 '24 23:07 miloush

Feature switches are used at runtime. They could probably be used at design-time, but that would be a misuse of feature switches IMO.

RussKie avatar Jul 31 '24 00:07 RussKie

You can learn more at https://learn.microsoft.com/dotnet/core/runtime-config/.

RussKie avatar Jul 31 '24 00:07 RussKie

  • I've tested the same stupid thing against .NET 8 and it will crash in runtime (when you set a resource ref) the same way it will crash with this PR in latest master, same spot, same call-stack; when running EvaluateExpression with InvalidPropertyValue

@h3xds1nz, regarding this, I would like to clarify the exact scenario. The application will not crash in .NET 8, if we do not include the brush SolidBackgroundFillColorSecondaryBrush (used here) directly in the project's resource dictionary (local) .

Instead if it is referenced from a resource dictionary not directly included with our application. In the repro, it is included via the Fluent dictionary here.

Since the above mentioned dictionary is not available in .NET 8, I have tweaked the above repro to load the wpf.ui styles which should make the difference between .NET 8 and .NET 9 more evident.

The mentioned behavior is present in the application here. The application is expected to launch in .NET 8 and crash with XamlParseException on .NET 9.

harshit7962 avatar Jul 31 '24 08:07 harshit7962

@h3xds1nz, regarding this, I would like to clarify the exact scenario. The application will not crash in .NET 8, if we do not include the brush SolidBackgroundFillColorSecondaryBrush (used here) directly in the project's resource dictionary (local) .

Sorry, it was a direct reply to miloush's question, when you try to set an invalid resource reference type during runtime on FrameworkElement f.e. The behaviour there is the same. As I've further continued, the only difference before and after PR is what we've confirmed, that is that the BAML/XAML parsing throws a XamlParseException already instead of it being swallowed.

h3xds1nz avatar Jul 31 '24 09:07 h3xds1nz

Can someone clarify what the migration expectations to newer versions of WPF are in general?

Actually, we would like to avoid breaking people as much as possible. And the places we would like to break are the ones that have good risk v/s rewards ratio.

Given the current state where applications having incorrect DynamicResource declaration would start to fail at runtime (where it didn't use to fail in earlier releases), this may create blocker for people wanting to migrate to .NET 9. The problem will be compounded if your app ends up using a library that provided a bad DynamicResource xaml.

We absolutely love this change, and we don't want to defer this change for next .NET release. As a stop-gap measure, we would be proceeding with adding an opt-out switch for .NET 9 release only.

We realize this is not ideal and having an opt-out for every single feature is certainly not the way we want our product to evolve. However, we are currently very close to release deadline for .NET 9 and I'm afraid that any more behavior changes at this point may create compat problems that we may not have enough time to fix.

pchaurasia14 avatar Aug 01 '24 12:08 pchaurasia14

@pchaurasia14 Thank you, being it an opt-out switch for .NET 9 release only is very much an acceptable solution that is consistent with the time constraints for .NET 9 in my opinion.

h3xds1nz avatar Aug 01 '24 14:08 h3xds1nz