Use IsEditorRequired in Equality checks

[pranavkm]

No description provided.

[SteveSandersonMS]

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

[TanayParikh]

#pranavPoints 😄

[SteveSandersonMS]

I guess you're fixing something specific here, but I can't guess what. I'm interested to know, but either way, the changes look totally reasonable.

[pranavkm]

I'm interested to know, but either way, the changes look totally reasonable.

I discovered it as part of https://github.com/dotnet/aspnetcore-tooling/pull/3798/files. I had initially thought that the property would necessarily matter for comparison, but there's some sort of caching that is performed by tooling: https://github.com/dotnet/aspnetcore-tooling/blob/main/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/TagHelperDescriptorCache.cs#L8 which tripped up a test.

[TanayParikh]

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);

[TanayParikh]

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

[pranavkm]

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

[TanayParikh]

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

[pranavkm]

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.

[TanayParikh]

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.

[pranavkm]

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