Skip to content

Use IsEditorRequired in Equality checks #33553

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Use IsEditorRequired in Equality checks #33553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
879fb66
Use IsEditorRequired in Equality checks
pranavkm Jun 15, 2021
045f4dc
Update BoundAttributeDescriptorComparer.cs
pranavkm Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Razor/Microsoft.AspNetCore.Razor.Language/src/BoundAttributeDescriptorComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

namespace Microsoft.AspNetCore.Razor.Language
{
internal class BoundAttributeDescriptorComparer : IEqualityComparer<BoundAttributeDescriptor>
internal sealed class BoundAttributeDescriptorComparer : IEqualityComparer<BoundAttributeDescriptor>
Copy link
Member

Choose a reason for hiding this comment

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

Seeing how much sealed shows up in @pranavkm's PRs, I propose renaming it to "the Pranav modifier".

Copy link
Contributor

Choose a reason for hiding this comment

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

#pranavPoints 😄

{
/// <summary>
/// A default instance of the <see cref="BoundAttributeDescriptorComparer"/>.
Expand All @@ -19,7 +19,7 @@ private BoundAttributeDescriptorComparer()
{
}

public virtual bool Equals(BoundAttributeDescriptor descriptorX, BoundAttributeDescriptor descriptorY)
public bool Equals(BoundAttributeDescriptor descriptorX, BoundAttributeDescriptor descriptorY)
{
if (object.ReferenceEquals(descriptorX, descriptorY))
{
Expand All @@ -37,6 +37,7 @@ public virtual bool Equals(BoundAttributeDescriptor descriptorX, BoundAttributeD
descriptorX.IsEnum == descriptorY.IsEnum &&
descriptorX.HasIndexer == descriptorY.HasIndexer &&
descriptorX.CaseSensitive == descriptorY.CaseSensitive &&
descriptorX.IsEditorRequired == descriptorY.IsEditorRequired &&
string.Equals(descriptorX.Name, descriptorY.Name, StringComparison.Ordinal) &&
string.Equals(descriptorX.IndexerNamePrefix, descriptorY.IndexerNamePrefix, StringComparison.Ordinal) &&
string.Equals(descriptorX.TypeName, descriptorY.TypeName, StringComparison.Ordinal) &&
Expand All @@ -48,7 +49,7 @@ public virtual bool Equals(BoundAttributeDescriptor descriptorX, BoundAttributeD
descriptorY.Metadata.OrderBy(propertyY => propertyY.Key, StringComparer.Ordinal));
}

public virtual int GetHashCode(BoundAttributeDescriptor descriptor)
public int GetHashCode(BoundAttributeDescriptor descriptor)
{
if (descriptor == null)
Comment on lines 53 to 54
Copy link
Contributor

Choose a reason for hiding this comment

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

Potentially worth adding this to ensure the TagHelper caching mechanism is able to differentiate between two similar taghelpers with different IsEditorRequired values.

hash.Add(descriptor.IsEditorRequired);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you'll need this to get around the caching as it's based on hash code and not equality. Disabling auto-merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh fun, I missed that. Do you know how it works for properties like IsEnum that aren't enumerated here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend adding in all uniquely identifiable properties into this GetHashCode. I tried to strike a balance between identifiable properties and simplicity (to ensure hashing doesn't add any substantial cost).

Hashing PR: #24551
Perf Benchmarks: dotnet/razor#2307

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But isn't the caching already busted at that point? In general though, it seems very problematic to use a hash to cache items. A hash isn't guaranteed to be unique. If tooling wanted to ensure this behavior, it should write it's own comparer that guarantees it's requirements.

Copy link
Contributor

@TanayParikh TanayParikh Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hashing is imperfect, but it was the tradeoff between accuracy and perf. Assuming enough entropy in the hash (by covering the subset of unique properties) chances of a hash collision should be minimal. Without this caching each GetTagHelpers call through Roslyn is going to cost ~230 tag helper allocations. And this is a fairly frequent call (IIRC occurs during compilations).

Going the comparator route would ensure accuracy but would undermine much of the perf benefits (as we'd still need to do a bunch of allocations and so on in order to subsequently do the comparisons on the tooling end). Right now if we match the hash within the cache, we skip deserializing/allocating the rest of the taghelper.

Copy link
Contributor Author

@pranavkm pranavkm Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right - but you get incorrect results with the current implementation. Correctness > perf.

{
Expand All @@ -58,6 +59,7 @@ public virtual int GetHashCode(BoundAttributeDescriptor descriptor)
var hash = HashCodeCombiner.Start();
hash.Add(descriptor.Kind, StringComparer.Ordinal);
hash.Add(descriptor.Name, StringComparer.Ordinal);
hash.Add(descriptor.IsEditorRequired);

if (descriptor.BoundAttributeParameters != null)
{
Expand Down
6 changes: 3 additions & 3 deletions src/Razor/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

namespace Microsoft.AspNetCore.Razor.Language
{
internal class TagHelperDescriptorComparer : IEqualityComparer<TagHelperDescriptor>
internal sealed class TagHelperDescriptorComparer : IEqualityComparer<TagHelperDescriptor>
{
/// <summary>
/// A default instance of the <see cref="TagHelperDescriptorComparer"/>.
Expand All @@ -19,7 +19,7 @@ private TagHelperDescriptorComparer()
{
}

public virtual bool Equals(TagHelperDescriptor descriptorX, TagHelperDescriptor descriptorY)
public bool Equals(TagHelperDescriptor descriptorX, TagHelperDescriptor descriptorY)
{
if (object.ReferenceEquals(descriptorX, descriptorY))
{
Expand Down Expand Up @@ -109,7 +109,7 @@ public virtual bool Equals(TagHelperDescriptor descriptorX, TagHelperDescriptor
}

/// <inheritdoc />
public virtual int GetHashCode(TagHelperDescriptor descriptor)
public int GetHashCode(TagHelperDescriptor descriptor)
{
if (descriptor == null)
{
Expand Down