Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multithreading performance #189

Open
Mahriel opened this issue Mar 5, 2019 · 18 comments
Open

Multithreading performance #189

Mahriel opened this issue Mar 5, 2019 · 18 comments

Comments

@Mahriel
Copy link
Contributor

Mahriel commented Mar 5, 2019

Hey guys

I encountered a performance issue with views in separated threads.
In a single threaded environment, the dialog takes about 15-20 seconds but when I’m starting the same dialog on a different thread it's bumping up to 65-70 seconds.
As you can see below the "OnProviderChanged" is taking 80% of the time.

wpflocalizationperformanceproblem

This dialog contains ~80 localized controls, the default values (dictionary, assembly, designculture) are set up like described in the wiki and i am using the latest version from nuget.

Could you maybe help me to solve this problem?

@xmedeko
Copy link
Contributor

xmedeko commented Mar 10, 2019

From a quick look into the code, I see the problem is in the ResxLocalizationProviderBase.OnProviderChanged method. @SeriousM @konne I do not understand the purpose of the code

var assembly = GetAssembly(target);
var dictionary = GetDictionary(target);
if (!string.IsNullOrEmpty(assembly) && !string.IsNullOrEmpty(dictionary))
    GetResourceManager(assembly, dictionary);

The result of the GetResourceManager is not used. Does the method have some side effects? Or is possible to remove this code? I've tried the DemoApplication without this code and it seems to work ok.

@SeriousM
Copy link
Collaborator

The result of the GetResourceManager is not used.

Where did you read that code, I can't find it in the current repo state.

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 10, 2019

The time consuming part is ProviderChanged?.Invoke(this, new ProviderChangedEventArgs(target));.
When it's commented out the start up time is fine but the localization doesn't work properly.

As far is i understand this event is refreshing all localized controls across threads.

@xmedeko
Copy link
Contributor

xmedeko commented Mar 11, 2019

@Mahriel can you hunt the root of the problem?

From your screenshot, I've though the problem is in the ParentChangedNotifierHelper.GetValueOrRegisterParentNotifier or maybe in the ParentChangedNotifierHelper.GetParent particularly? They are called from GetResourceManager.

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 11, 2019

When GetResourceManager is called with a unknown dictionary it takes 300-400ms on the first time. Afterwards the call takes <1ms.

But on every call the ProviderChanged?.Invoke needs 700-1000ms and the OnProviderChanged is called 60 times.
While constructing the view the first 4 call's are handled and this should be fine because i am setting a DefaultAssembly & DefaultDictionary on the view and a usercontrol. As soon as Window.Show() is called it's refreshing every control again?
Here's a sample callstack from on of those OnProviderChanged when Show is called:

WPFLocalizeExtension.dll!WPFLocalizeExtension.Extensions.LocExtension.ResourceChanged(System.Windows.DependencyObject sender, WPFLocalizeExtension.Engine.DictionaryEventArgs e) Line 322 C#
WPFLocalizeExtension.dll!WPFLocalizeExtension.Engine.LocalizeDictionary.DictionaryEvent.Invoke(System.Windows.DependencyObject sender, WPFLocalizeExtension.Engine.DictionaryEventArgs args) Line 1005 C#
WPFLocalizeExtension.dll!WPFLocalizeExtension.Engine.LocalizeDictionary.ProviderUpdated(object sender, WPFLocalizeExtension.Providers.ProviderChangedEventArgs args) Line 199 C#
WPFLocalizeExtension.dll!WPFLocalizeExtension.Providers.ResxLocalizationProviderBase.OnProviderChanged(System.Windows.DependencyObject target) Line 601 C#
WPFLocalizeExtension.dll!WPFLocalizeExtension.Providers.ResxLocalizationProvider.ParentChangedAction(System.Windows.DependencyObject obj) Line 242 C#
WPFLocalizeExtension.dll!WPFLocalizeExtension.Providers.ParentChangedNotifierHelper.GetValueOrRegisterParentNotifier.AnonymousMethod__0() Line 110 C#
XAMLMarkupExtensions.dll!XAMLMarkupExtensions.Base.ParentChangedNotifier.ParentChanged(System.Windows.DependencyObject obj, System.Windows.DependencyPropertyChangedEventArgs args) Line 68 C#
WindowsBase.dll!System.Windows.DependencyObject.OnPropertyChanged(System.Windows.DependencyPropertyChangedEventArgs e) Unknown
PresentationFramework.dll!System.Windows.FrameworkElement.OnPropertyChanged(System.Windows.DependencyPropertyChangedEventArgs e) Unknown

Edit:
When the application is executed on a single thread the behavior stays the same but the ProviderChanged?.Invoke only takes <10ms

@xmedeko
Copy link
Contributor

xmedeko commented Mar 11, 2019

So the problem is in LocExtension.ResourceChanged ? Can you track where exactly it hangs for 700-1000ms?

So, when running on a single thread then the delay is acceptable?

Try to remove all lines I have mentioned from OnProviderChanged, especially those GetAssembly(target) and GetDictionary(target). They add notifiers during the view initialization. I think it should limit the number the OnProviderChanged is called later.

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 11, 2019

Yes on a single thread the delay is fine.

Removing GetAssembly, GetDictionary and GetResourceManager doesn't make any difference.

When you look at the Callstack (sorry for the bad format) you can see the event is triggerd by WPF DependencyObject.OnPropertyChanged -> XAMLMarkupExtensions ParentChangedNotifier.ParentChanged -> down to the ResxLocalizationProviderBase.OnProviderChanged.

So i had a closer look at the LocExtension.ResourceChanged it's called for 45 items. The most calls are below 1ms in execution time.
But i have 2 deeply nested items in this collection and at this point we are loosing time. It takes 300 - 500 ms each ResourceChanged call.
Both items have 226 targetDOs and for each element the while is executing 50 and 52 times.

@xmedeko
Copy link
Contributor

xmedeko commented Mar 11, 2019

AFAIK Each Loc tries to walk up the element tree and search for the nearest DefaultAssembly and DefaultDictionary. If not found, the topmost element is registered for the ParentChanged event and upon this event the search is performed again.
Do you have DefaultAssembly and DefaultDictionary set in every XAML?

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 11, 2019

Every window and usercontrol has its own resx file so i'm setting one or both options.

@xmedeko
Copy link
Contributor

xmedeko commented Mar 11, 2019

Try to set both DefaultAssembly and DefaultDictionary in every XAML which uses WPFLocalizationExtension. And then try to keep or remove GetAssembly, GetDictionary GetResourceManager from ResxLocalizationProviderBase.OnProviderChanged.

@konne
Copy link
Member

konne commented Mar 12, 2019

@Mahriel it would be helpfull, if you add in a second PR a simple app with like 2 Windows / Threads to show the issue. When we can also debug this issue.

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 19, 2019

@konne @xmedeko did you have time to check the test app?

@konne
Copy link
Member

konne commented May 27, 2020

@Mahriel I know that more than one year is over, but I started now to cleanup this project.
Are you still interested to follow up on this topic?
It would be great to have a skype / ... meeting to discuss and see what can be done.
In the meantime we did also some performance improvements in the underlying XAMLMarkupExtensions Lib.

@konne
Copy link
Member

konne commented Aug 17, 2020

@Mahriel PING?!?

@Mahriel
Copy link
Contributor Author

Mahriel commented Sep 8, 2020

Hello @konne sorry for the late reply.
We are currently using a custom build with the changes from my PR. We've also changed a lot on our framework side so i don't know exactly if the performance issue still exists with the official WPFLocalizationExtension release.
My boss won't give me time to carry on the investigation on this topic.
Feel free to contact me anyway, maybe we can find a solution in another way.

@ciuckc
Copy link

ciuckc commented Aug 26, 2024

Hi any news about this?

@Karnah
Copy link
Contributor

Karnah commented Sep 30, 2024

I did some small research on this problem.
Let us have two UI threads: thread1 and thread2. Both thread objects are stored together in one list - DictionaryEvent.Listeners. When we do some operation in thread1, we also check elements which belongs thread2 and for each of these elements, we call element.Dispatcher.Invoke(...) many times (check parent, get or set value, etc). This method is calling dozens of time, and it quite slow.

The solution of @Mahriel makes sense: he is grouping objects by thread and calling element.Dispatcher.Invoke(...) only once per thread. This is much faster. But it should be tested very carefully, because multithreading usually have a lot if different hidden problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants