Ensure we cache the previous item when two items are considered equal#57888
Conversation
You say "We" here but is this the way the Razor team is designing their comparers / state?
I agree in general you should prefer "previous" over the "new" one but given a correct implementation of comparison this shouldn't be matter. I'm still a bit unclear what the problem is here. Based on the code change my intuition is that Razor has a non-conformant comparer here. Am I missing a subtlty of the change? |
|
@jaredpar Yeah, its kind of an odd one. Ultimately the comparer that the razor team is breaking transitivity. In a simpler example, if we have a comparer where Current: Fixed: While this might mean we occasionally run when we don't need to (if the user has a dud comparer) we'll no longer be in the situation where we should run, but didn't, which can lead to broken code. |
Do we have a bug on the Razor team for them to fix this? In theory I agree keeping previous is better than new as a general rule in the cache (better for GC purposes as we're promoting less values to higher generations). Hence I'm okay with the change overall. That should be an tiny performance detail though, not a correctness one. Doing this for correctness feels wrong, it's not a bug in our implementation, it's a bug in Razor they should be fixing in parallel. Any non-transitive comparer here is going to cause problems. |
|
I'm not sure if there's a single bug for razor. The fix is to unify the design time and build time experiences, until they do that they need to be able to compare in the way that they're doing to achieve any sort of perf. Agree we shouldn't encourage it, but my point was with this approach a broken comparer produces correct code slowly, rather than wrong code quickly. |
|
The explanation at #57888 (comment) made the issue immediately obvious. Thanks. |
| AssertTableEntries(previousTable, expected); | ||
|
|
||
| builder = previousTable.ToBuilder(); | ||
| Assert.True(builder.TryModifyEntry(1, EqualityComparer<int>.Default)); // ((1, EntryState.Cached)) |
There was a problem hiding this comment.
So I keep forgetting: is the entry we're trying to modify denoted implicitly by the order of calls to TryModifyEntry?
|
|
||
| var entryState = comparer.Equals(previous, replacement) ? EntryState.Cached : EntryState.Modified; | ||
| modified.Add(replacement, entryState); | ||
| (var chosen, var state) = GetModifiedItemAndState(previous, replacement, comparer); |
There was a problem hiding this comment.
nit: can be var (chosen, state) ;)
| { | ||
| // when comparing an item to check if its modified we explicitly cache the *previous* item in the case where its | ||
| // considered to be equal. This ensures that subsequent comparisons are stable across future generation passes. | ||
| return comparer.Equals(previous, replacement) |
There was a problem hiding this comment.
I wouldn't dive into any rabbit holes here but I do wonder if there is a practical way to observe that the user's comparer is not transitive and give a warning. Seems like whatever you could do to check that would introduce undesirable overhead in a release build, though.
|
Is it cool if I merged this change since it's ready to go? I think @chsienki is out for a couple more days and I'd like to give this change a try before I have to take off for the holidays. |
|
Sure. |
|
@pranavkm can you link me the bug on the razor side tracking that the comparer you're using has incorrect semantics? That seems like a bigger issue here that we need to track down. |
Fixes a subtle issue for Razor:
When the razor generator is running in 'suppressed' mode, they effectively treat all comparisons as being true, so that the driver just uses what is in the cache without re-calculating anything. Today, in the generator driver we take the new item and place it in the cache when this happens. For compound objects, this can mean that a comparer is not stable across iterations.
Consider:
We have the following compound object:
(bool check, object state)With a comparer like:
(old, @new) => @new.check || old.state.Equals(new.State)In the first run the values are
(false, null)and we have no cache yet, so the comparer will be ignored and we write into the state table:(false, null)and the generator logic runs, producing downstream results from the(false, null)values.In the second iteration we have
(true, object1). The comparer will run, return true, and the driver will write in the new state of(true, object1). However because the comparer returned true, the actual driver logic is not invoked and the cached values of the previous run are used.In the third iteration we have
(false, object1). The comparer will run, and use the second branch of the logic: however, we will be comparingobject1toobject1which will return true. Once again the cached version of the results are used which were generated from(false, null)which is incorrect.Instead when a comparer returns that two object are equal, we return the original item to the cache.
States:
Incorrect:
(false, null)null(false, null)(true, object1)(false, null)(false, null)(false, object1)(true, object1)(false, null)Fixed:
(false, null)null(false, null)(true, object1)(false, null)(false, null)(false, object1)(false, null)(false, object1)