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

Optimize WeakDelegateReference by introducing TargetEquals, which tak… #1793

Merged
merged 2 commits into from
May 24, 2019

Conversation

hermestobias
Copy link
Contributor

@hermestobias hermestobias commented May 10, 2019

…es 60% less time than comparing with Target if it's still alive, and makes WeakDelegateManager.RemoveListener take 90% less time in normal usage.

### Description of Change ###

In our application, we saw a lot of time being spent inside WeakDelegateManager.RemoveListener when injecting a view with many subviews (around 100 in total). This was due to WeakDelegateManager creating temporary Delegates for each DelegateReference in the list just to do comparisions. By introducing TargetEquals method, you can now test equality without actually using. Testing the fix locally showed that time spent in WeakDelegateManager.RemoveListener got reduced with 90% and made our view injection almost 3 seconds faster!

Bugs Fixed

#1796

API Changes

List all API changes here (or just put None), example:

Added:

  • bool DelegateReference.TargetEquals(Delegate);

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

…es 60% less time than comparing with Target if it's still alive, and makes WeakDelegate.RemoveListener take 90% less time in normal usage.
@dnfclas
Copy link

dnfclas commented May 10, 2019

CLA assistant check
All CLA requirements met.

@brianlagunas
Copy link
Member

Thank you for the PR, but I wish you would have submitted an issue first. These percentages need to be verified before I can merge this PR. Do you have any sample/test apps to confirm your statements? Specifically, I need to see the perf before the change was made, and then again after the changes are made. Then we can look at the numbers and calculate the exact percentage of improvement, if any exists.

@hermestobias
Copy link
Contributor Author

I have only profiled our full closed source application (which is using Prism 6.3, so did the change there initially and rebased later).

I guess I can make a test application (in 7.X) to show the performance gains this optimization provides. Do you want me to provide that in form of an issue, and then refer to it from this?

@brianlagunas
Copy link
Member

Yes please create a new issue, provide your findings, and link back to this PR. Thanks.

@hermestobias
Copy link
Contributor Author

But it replaces expensive calls to CreateDelegate with GetMethodInfo, which it then can avoid for most cases where operands do not have the same target (which you can see are the majority of cases in WeakDelegateManager). But seeing is believing I guess...

@hermestobias
Copy link
Contributor Author

This PR is a fix for #1796

@lock
Copy link

lock bot commented Jan 28, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants