[Routing] Proactively checks constraints and complex parameters when building the DFA tree to trim unnecessary nodes #33712
Conversation
| public interface ILiteralConstraint : IParameterPolicy | ||
| { | ||
| /// <summary> | ||
| /// Determines whether the given <paramref name="literal"/> can match the constraint. | ||
| /// </summary> | ||
| /// <param name="parameterName">The parameter name we are currently evaluating.</param> | ||
| /// <param name="literal">The literal to test the constraint against.</param> | ||
| /// <returns><c>true</c> if the literal contains a valid value; otherwise, <c>false</c>.</returns> | ||
| bool MatchLiteral(string parameterName, string literal); | ||
| } |
There was a problem hiding this comment.
Need to come up with a better name for this, in essence we want to implement this on our constraints and let people implement it too. Other names are IParameterNodeLiteralPolicy or IParameterNodeBuilderPolicy.
The idea here is that we pass literal values when we are building the DFA tree and you get to decide whether they match or not.
We also pass you the parameter name so that you can implement policies based on the parameter name even if you don't know the values ahead of time. For example, if the parameter name is controller or page, you can choose to never add any literal from the tree to mean something like, this parameter will never match any value from the list of literals in the tree.
There was a problem hiding this comment.
I'm going to move this from the abstractions assembly to the implementation assembly, since this is mostly a DFA concern and rename it to IRouteParameterNodeFilterPolicy
|
Issue #? Who do you want to review this? I don't think Brennan or I have the context. |
| [assembly: TypeForwardedTo(typeof(EndpointMetadataCollection))] | ||
| #pragma warning disable RS0016 | ||
| [assembly: TypeForwardedTo(typeof(RouteValueDictionary))] | ||
| #pragma warning restore RS0016 |
There was a problem hiding this comment.
VS and the build were complaining about this
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
A review focusing on code.
I'll wait until we've talked through this before thinking about the overall solution.
javiercn
force-pushed
the
javiercn/routing-dfa-tree-trimming
branch
from
25457b1 to
85cc2ec
Compare
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
New files need MIT license headers
|
Is this change going in @javiercn ? |
|
@davidfowl RC1 |
|
@davidfowl @JamesNK any further concern here? I want to go through public API review with this today. |
javiercn
added
the
api-ready-for-review
|
Thank you for your API proposal. I'm removing the |
ghost
removed
the
api-ready-for-review
javiercn
force-pushed
the
javiercn/routing-dfa-tree-trimming
branch
from
3bcb6a9 to
89bd1df
Compare
| [assembly: TypeForwardedTo(typeof(EndpointMetadataCollection))] | ||
| #pragma warning disable RS0016 | ||
| [assembly: TypeForwardedTo(typeof(RouteValueDictionary))] | ||
| #pragma warning restore RS0016 |
There was a problem hiding this comment.
I'll check for this and try out removing the #pragma
src\Http\Routing.Abstractions\src\Properties\AssemblyInfo.cs(13,12): error RS0016: Symbol 'TryAdd' is not part of the declared API. []...Microsoft.AspNetCore.Routing.Abstractions.csproj
javiercn
force-pushed
the
javiercn/routing-dfa-tree-trimming
branch
from
b632ffb to
8fafc9f
Compare
amcasey
added
area-networking
Due to the way we construct our DFA tree, several route patterns where there is a variable at the beginning of the route pattern in addition to a large set of different literals as the first segment of the route tree, results in massively large route tables (GB and more) many of them filled with nodes that we don't need.
The reason we don't need those nodes is because the literal they are hanging from won't match the constraint of the parameter or the shape of the complex parameter (or a constraint in a complex parameter part), hence even if we get to a candidate for that pattern, it will be discarded when we check constraints and complex parameters.
Instead of adding those nodes when building the DFA tree and evaluating the constraints and complex parameters on each request we evaluate the constraints and the complex parameters as we are building the DFA by evaluating the parent literal candidate against the constraint/complex parameter.
This results in DFAs with a much smaller number of nodes (several orders of magnitude depending on the case) and improves memory as well as startup time on these cases.