[Release/3.1] ReplacingEV uses two arrays instead of dictionary (#19858)

[roji]

ReplacingExpressionVisitor held original and replacement expressions in a dictionary. Unfortunately that means hash code calculation occurs exponentially, as each TryGetValue on the dictionary must traverse the entire tree.

Replaced with two lists and a simple Equals check, which is much faster.

Fixes #19737

(cherry picked from commit 7c9fc8a)

[roji]

Description

When some operators (e.g. Where) are composed over a deep, self-referencing LINQ query tree, hash code calculation can make translation time exponential and make compilation grind to a halt.

Customer Impact

Query compilation can hang for extremely long times before even sending a SQL query to the database.

How found

Reported by a customer.

Test coverage

Perf issue, only produced by some specific operators (e.g. Where) composed over a deep, self-referencing query tree.

We will add a test for this scenario to our perf tests.

Regression?

Query code has changed significantly since 2.x, so this particular case may or may not be a regression.

Risk

Low. Controlled, pointed change to a simple expression visitor which replaces nodes in the tree.

Also, fix is on by default, but quirk switch is present to revert back to old behavior.

[ajcvickers]

@roji Can you "quirk" the fix. That is, put the change behind an `AppContext.TryGetSwitch" like Smit has done here: https://github.com/dotnet/efcore/pull/19871/files#diff-baecacd4cd9354c21b22e390097a8d79R511

The new behavior should be enabled by default. When the quirk switch is set, then we should revert to the pre-patch behavior. This allows customers to switch back to the old behavior if we accidentally broke something. These switches never make it into master--they are specifically there for the patches only.

[roji]

@ajcvickers done, though this is a bit more complicated then just one condition. Tested that with and without quirks things work as expected, but would be good to get another pair of eyes.

[ajcvickers]

@roji Can you create an issue to add this scenario to the perf tests? We should then have some coverage that this doesn't regress.

[roji]

I was just thinking that, opened #19908.

[ajcvickers]

@roji @smitpatel Approved by Tactics and branches are open; let's try to get this merged today ASAP since it's already night for you. Not that that seems to matter. ;-)