Skip to content

Instance per Thread#190

Open
Mahriel wants to merge 4 commits intoXAMLMarkupExtensions:developmentfrom
Mahriel:feature/threadInstance
Open

Instance per Thread#190
Mahriel wants to merge 4 commits intoXAMLMarkupExtensions:developmentfrom
Mahriel:feature/threadInstance

Conversation

@Mahriel
Copy link
Contributor

@Mahriel Mahriel commented Mar 8, 2019

#189

When it comes to bigger ui multi-threaded applications the WPFLocalizationExtension is losing a lot of performance on cross thread calls while ILocalizationProvider.ProviderChanged is called.

Therefor I implemented a new feature to use an instance per thread and not a singleton instance.

As long as LocalizeSettings.Instance.UseThreadInstances isn't activated the program will do the same as before and use a singleton instance.
When the flag is activated you'll get a instance per thread for the LocalizeDictionary and all Providers. You should be using the LocalizeSettings.Instance instead of LocalizeDictionary.Instance to change settings, like the current culture, for all instances.

Implemented InstanceLocator to get singleton or per thread instances.
Adapted all Providers and Dictionary to use the new instance logic.
@konne
Copy link
Member

konne commented Mar 8, 2019

@Mahriel thansk for this PR, please can you just fix the 4 small new Codefactor issues.
I try to have a look at your PR next week.

@konne
Copy link
Member

konne commented Mar 8, 2019

@SeriousM @xmedeko please can you give your feedback

@xmedeko
Copy link
Contributor

xmedeko commented Mar 10, 2019

Is this change necessary or we better clean and fix existing code? See my comment in #189

@SeriousM
Copy link
Collaborator

SeriousM commented Mar 10, 2019

@Mahriel:

  • could you explain us why your code is faster in a multi-thread application? It looks like you found a solution to your problem here.
  • please change your implementation to not rely on static all over the place
  • please avoid an implementation that does A OR B. implement two services with different behaviors and then call either the one or the other, depending on a setting/option

@konne: the PR is not mature enough and doesn't contain even a single test. to me this is a big untested/unproven change and we should stay away from it

@Mahriel
Copy link
Contributor Author

Mahriel commented Mar 10, 2019

@SeriousM

  • see comment
    This approach is preventing the cross thread calls but maybe there's a better solution. My boss didn't give me enough time to investigate the real issue. it just had to work till friday :(
  • Do you mean the implementation of the InstanceLocator?
  • So for example i should create a ResxLocalizationThreadProvider and change the implementation of the static Instance property?

@CLAassistant
Copy link

CLAassistant commented Apr 27, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@SeriousM
Copy link
Collaborator

@konne what is this cla assistant? We have another license than the one pointed out in the link.
It says "Fiduciary License Agreement 2.0 based on the
Individual Contributor exclusive License Agreement."
Why do we need this?

Instance = null;
var instance = Instance;
InstanceLocator.Dissolve(instance);
instance = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me should be Instance = null

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

Successfully merging this pull request may close these issues.

6 participants

Comments