-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Improve performance of, and reduce allocations in, UIHintAttribute #57392
Conversation
…by avoiding both Linq and interface calls.
Tagging subscribers to this area: @ajcvickers, @bricelam, @roji Issue DetailsImproves performance of, and reduces allocations in, System.ComponentModel.DataAnnotations.UIHintAttribute by avoiding both System.Linq and interface calls. This affects the This is also a first (small) step toward removing the use of System.Linq in System.ComponentModel.Annotations. Benchmarks
Benchmark Code [MemoryDiagnoser]
public class UIHintAttributeBenchmarks
{
private UIHintAttribute _hint1;
private UIHintAttribute _hint2;
private UIHintAttribute _hint3;
private object[] _controlParametersSmall;
[GlobalSetup]
public void GlobalSetup()
{
_hint1 = new UIHintAttribute("abc", "def", new object[] { "A", 1, "C", 3, "B", 2 });
_hint2 = new UIHintAttribute("abc", "def", new object[] { "B", 2, "A", 1, "C", 3 });
_hint3 = new UIHintAttribute("abc", "def", new object[] { "B", 2, "A", 1, "D", 4 });
_controlParametersSmall = new object[] { "A", 1, "B", 2, "C", 3 };
}
[Benchmark]
public bool Equals_HintAndPresentationLayerAndControlParametersAreEqual()
{
return _hint1.Equals(_hint2);
}
[Benchmark]
public bool Equals_ControlParametersAreNotEqual()
{
return _hint1.Equals(_hint3);
}
[Benchmark]
public System.Collections.Generic.IDictionary<string, object> ControlParameters_Empty()
{
var hint = new UIHintAttribute("abc", "def", Array.Empty<object>());
return hint.ControlParameters;
}
[Benchmark]
public System.Collections.Generic.IDictionary<string, object> ControlParameters_Small()
{
var hint = new UIHintAttribute("abc", "def", _controlParametersSmall);
return hint.ControlParameters;
}
}```
<table>
<tr>
<th align="left">Author:</th>
<td>drieseng</td>
</tr>
<tr>
<th align="left">Assignees:</th>
<td>-</td>
</tr>
<tr>
<th align="left">Labels:</th>
<td>
`area-System.ComponentModel.DataAnnotations`, `community-contribution`
</td>
</tr>
<tr>
<th align="left">Milestone:</th>
<td>-</td>
</tr>
</table>
</details> |
} | ||
if (inputControlParameters.Length % 2 != 0) | ||
{ | ||
throw new InvalidOperationException(SR.UIHintImplementation_NeedEvenNumberOfControlParameters); | ||
} | ||
|
||
Dictionary<string, object?> controlParameters = new Dictionary<string, object?>(inputControlParameters.Length / 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this size a good estimate? If everything was a dup, this could end up allocating way more than before, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not an estimate. The inputControlParameters contains two entries for each kvp that we'll add to the dictionary.
We'll indeed allocate more if a key is null, not a string or a duplicate.
I assumed that it would be ok to focus on the happy path, but (I should not have assumed anything and) we do not gain much from this.
Let me know if you want me to revert this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that it would be ok to focus on the happy path
I've never actually used this type. Is that the 99.9% case? Can you point to examples of it being used? I'm curious how this type shows up in your own code that led you to submit this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I have never use this type. I just follow all commits in this repo, and I saw another commit that touched this assembly and left room for improvement. This lead me to preparing some other improvements (of which this is just one, others to follow).
I was perhaps too optimistic about the happy path being the common path, I'll just revert this part of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. If the code isn't hot path, I'd actually prefer we not try to remove the simple LINQ usage by creating open-coded replacements for it. Or are there other reasons for removing the LINQ dependency? Is there some usage somewhere of this type that highlights why this is valuable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just one of the few uses of Linq in this assembly, and I just thought it may be a good idea to eliminate the use of Linq altogether here (for both perf and allocations). I'll just close the PR then.
} | ||
|
||
return controlParameters; | ||
} | ||
|
||
private static bool ControlParametersAreEqual(Dictionary<string, object?> left, Dictionary<string, object?> right) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should assert that the comparers are what we expect them to be. This implementation depends on them being the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the same comparer that the original Linq implementation used (unless I'm mistaken). Let me know what you want me to verify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That left.Comparer
== right.Comparer
== EqualityComparer<object>.Default
. If those differ, this routine may not do the correct thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only use EqualityComparer<object>.Default
to compare the values, not for the dictionary. I did not change the comparer of the dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. I'm saying that you've added a utility method here that depends on other code creating the dictionaries in a certain way in order for this code to work correctly. As such this code should assert they were created correctly.
Improves performance of, and reduces allocations in, System.ComponentModel.DataAnnotations.UIHintAttribute by avoiding both System.Linq and interface calls. This affects the
Equals(object? obj)
method and theControlParameters
property.This is also a first (small) step toward removing the use of System.Linq in System.ComponentModel.Annotations.
Benchmark Results
Benchmark Code