Small performance improvements from feedback traces#51850
Small performance improvements from feedback traces#51850sharwell merged 3 commits intodotnet:mainfrom
Conversation
Addresses 83% of allocations within IncrementalAnalyzerProcessor.Enqueue.
| AssemblyIdentityParts parts; | ||
|
|
||
| if (reference != null) | ||
| if (reference is not null) |
There was a problem hiding this comment.
📝 The != here was an overloaded operator, and the performance trace indicated a non-trivial amount of time was spent in Compare before entering TriviallyEquivalent. I'm not absolutely certain the interpretation is correct, but it's easy to update this null check to ensure there is no unnecessary complexity on the hot path.
There was a problem hiding this comment.
This rather surprises me. The overloaded operators here simply call EqualityComparer<AssemblyIdentity>.Default.Equals, which should be quick when one is null and one is not. If we don't think this is true, then we should make a change the operator to make reference equality quicker, rather than making a change here.
In reply to: 593474596 [](ancestors = 593474596)
There was a problem hiding this comment.
The overloaded operators here simply call EqualityComparer.Default.Equals, which should be quick when one is null and one is not. If we don't think this is true, then we should make a change the operator to make reference equality quicker, rather than making a change here.
It is always good to bypass a method call for a simple null check. If an improvement can be made to implementation of the operator then we should probably handle that separately. However, we usually do not optimize implementation of equality operators for the purpose of null checks. For reference equality - yes, but not for null checks. The implementation of Equals looks reasonable:
public bool Equals(AssemblyIdentity? obj)
{
return !ReferenceEquals(obj, null)
&& (_lazyHashCode == 0 || obj._lazyHashCode == 0 || _lazyHashCode == obj._lazyHashCode)
&& MemberwiseEqual(this, obj) == true;
}
In reply to: 594677769 [](ancestors = 594677769,593474596)
There was a problem hiding this comment.
@333fred ReproTrace4.etl.zip attached to the linked issue shows what I was looking at here. It's hard to be certain but based on machine code offsets in the stack trace it looks like the hot paths in Compare precede the call to TriviallyEquivalent, and there just isn't much in the method before that call.
There was a problem hiding this comment.
In the absence of a good way to micro-benchmark this method using the same inputs from that feedback, this change seemed to fall under "tolerable guesswork".
|
Done review pass (commit 3) |
Closes AB#1279909