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

Optimized work with Listener list #308

Conversation

Karnah
Copy link
Contributor

@Karnah Karnah commented Aug 19, 2021

Listeners list has the same problems which had NestedMarkupExtension:

  1. AddListener was O(N) + constant for deleting weak refernces + memory using for 'ToList()' call. Now - O(lgN).
  2. RemoveListener was O(N) + constant for deleting dead weak refernces + memory using for 'ToList()' call. Now - O(lgN).
  3. Invoke and EnumerateListeners has problem with memory ('ToList()' call) + deleting dead weak refernces.

So, I hope this changing will increase perfomence on all operations with listener list.

p.s. Now Invoke method all executing under lock. It seems to me and I hope this won't have some bad effects

@Karnah
Copy link
Contributor Author

Karnah commented Aug 24, 2021

I found an interesting problem. listener.ResourceChanged can throw an exception. Usually, I met this when there were some problems in markup (for example error when we cannot change Binding after it has been initialized).
If this error happens now - listerList will not be cleared from dead listeners. I cannot just move ClearDeadReferences to finally block because then it will not delete dead references after the listener throws an exception.
There is another old problem. If one of the listeners threw an exception - for rest listeners will not call ResourceChanged. I think this is not so good too. Can I always collect all exceptions from listeners and then throw one AggregatedException?

Copy link
Member

@konne konne left a comment

Choose a reason for hiding this comment

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

@Karnah please see comment

src/Engine/LocalizeDictionary.cs Outdated Show resolved Hide resolved
@konne
Copy link
Member

konne commented Aug 24, 2021

If this error happens now - listerList will not be cleared from dead listeners. I cannot just move ClearDeadReferences to finally block, because then will not delete dead references after the listener which throws an exception.

Can we introduce some kind of a global static list there References with these Exceptions are added and then in later runs can be just deleted. This is not perfect but a little bit like the .net garbage collection.

There is another old problem. If one of the listeners threw an exception - for rest listeners will not call ResourceChanged. I think this is not so good too. Can I always collect all exceptions from listeners and then throw one AggregatedException?

I think an AggregatedException makes sense.

@Karnah TIP for you. Install Grammarly to help you fix most of the English errors in these comments even with the unpaid version.

@Karnah
Copy link
Contributor Author

Karnah commented Aug 24, 2021

Can we introduce some kind of a global static list there References with these Exceptions are added and then in later runs can be just deleted. This is not perfect but a little bit like the .net garbage collection.

I have an idea to make ClearDeadReferences private and always call it at the beginning of AddListener, GetListeners, and RemoveListener. Dead references will live a bit longer, but external code may not worry about a moment when we should call this.

* ClearDeadReferences is called only inside ListenersList
Copy link
Member

@konne konne left a comment

Choose a reason for hiding this comment

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

@Karnah please check the comments

src/Engine/ListenerList.cs Show resolved Hide resolved
/// </summary>
public void AddListener(IDictionaryEventListener listener)
{
ClearDeadReferences();
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense to call it here? because it does not search for new dead references like in GetListeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it, because of a subsequence of operations.

  1. Call GetListeners -> it creates list of dead references
  2. Call any other method -> references are removed before main operation

But I found a better solution which is not obvious. If wrap foreach (var listener in listeners) with try/finally and call ClearDeadReferences in finally block -> reference will be always deleted after this method call, even if calling code use break;

Copy link
Member

Choose a reason for hiding this comment

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

can you push this change, with the "finally". That sounds great to me.

@konne konne merged commit cf693bb into XAMLMarkupExtensions:development Aug 26, 2021
@Karnah Karnah deleted the feature/optimize-work-with-listeners-list branch August 27, 2021 18:08
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.

2 participants