Skip to content

Small performance improvements from feedback traces#51850

Merged
sharwell merged 3 commits intodotnet:mainfrom
sharwell:small-perf-fixes
Mar 19, 2021
Merged

Small performance improvements from feedback traces#51850
sharwell merged 3 commits intodotnet:mainfrom
sharwell:small-perf-fixes

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Mar 12, 2021

Closes AB#1279909

@sharwell sharwell requested review from a team as code owners March 12, 2021 21:57
@ghost ghost added the Area-IDE label Mar 12, 2021
AssemblyIdentityParts parts;

if (reference != null)
if (reference is not null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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".

@333fred
Copy link
Member

333fred commented Mar 15, 2021

Done review pass (commit 3)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@sharwell sharwell merged commit 595de20 into dotnet:main Mar 19, 2021
@sharwell sharwell deleted the small-perf-fixes branch March 19, 2021 04:06
@ghost ghost added this to the Next milestone Mar 19, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants