Remove rewrite rule API with default assumptions about skip remaining rules

[natemcmaster]

Changes an important, common use-case. When defining rewrite rules explicitly, the .Add(string regex, string replacement) API should default to setting skipRemainingRules = true.

Consider the following code and graph:

var option = new RewriteOption();
// where N is the x-axis on the graph
for(var tenant = 1; tenant <= N; tenant++) 
{
   option.Add($"app/{tenant}/(.*)", "/common/$1?tenantid=" + tenant);
}

When skipRemainingRules=false, we get the the performance shown on the orange line. This is a perf hit of -75% raw throughput for all requests, not just the rewritten ones.

With skipRemainingRules=true (proposed default), we get the the performance shown on the yellow line (only ~7% perf hit).

The overload .Add(string regex, string replacement, bool skipRemainingRules) is still available for the less-common case of wanting chained rewrite rules.

cc @Tratcher @mikaelm12

[Tratcher]

Note that's the opposite of UrlRewrite and ModRewrite and may not match customer expectations.

[natemcmaster]

may not match customer expectations.

I don't have any data on what customers would expect by default from the API, but I think the perf pit of failure is big enough to warrant the change regardless.

[Tratcher]

Your test case also assumes the first rule matches and short circuits. On average you'll have to go through at least half of the rules.

[mikaelm12]

Yeah, not sure how I feel about defaulting to skipping the remaining rules but that chart is pretty scary.

[natemcmaster]

I've added the average case (green line)

[natemcmaster]

Another proposal: we remove the .Add(string,string) API altogether which requires callers to explicit opt-in/opt-out to the "skipRemainingRules" behavior. Thoughts @glennc @davidfowl @muratg @Eilon ?

[mikaelm12]

I like this more than skipping by default.

[muratg]

Removing the overload seems reasonable to me too.

I'd like to see the perf impact with 20-30 rules and see if it's impactful enough.

[Eilon]

BTW did we already do the thing about $ and ^ in the regex? Because I suspect that could itself have a huge impact on perf and affect all other perf decisions.

[muratg]

Do you mean adding those characters to the regex str (if they're not already there?)

[glennc]

I am ok with adding an overload later if we don't know what we should be defaulting it to right now. We can look at what people say and add the overload later if people want it.

[natemcmaster]

@Eilon did we already do the thing about $ and ^ in the regex?

It's the light gray line in this chart above: https://github.com/aspnet/BasicMiddleware/pull/105#issuecomment-248717373 Compare with the blue line. It makes very little difference.

@glennc I am ok with adding an overload later if we don't know what we should be defaulting it to right now. We can look at what people say and add the overload later if people want it.

:+1: I'll update this PR to just remove the overload then.

[natemcmaster]

:up: :date:

[mikaelm12]

Looks good to me 👍

[troydai]

Looks good to me.

[Tratcher]

IIS and Apache chain by default.

[natemcmaster]

I think this is worth discussing again after we do more perf improvements. Opened https://github.com/aspnet/BasicMiddleware/issues/112. But i'm gonna merge this as it's easier to give API than to take away.