Link Generator perf improvements. #788
Conversation
| // The uncommon case is that the propertyStorage is in use | ||
| if (dictionary._propertyStorage == null && ((uint)_index < (uint)dictionary._count)) | ||
| { | ||
| Current = _dictionary._arrayStorage[_index]; |
There was a problem hiding this comment.
Should this use the local variable? dictionary._arrayStorage[_index];
If there is a reason not to then explain in a comment
There was a problem hiding this comment.
ooh spicy, yeah this is a mistake.
| var dictionary = _dictionary; | ||
| if (dictionary._propertyStorage != null && ((uint)_index < (uint)dictionary._count)) | ||
| { | ||
| var storage = _dictionary._propertyStorage; |
There was a problem hiding this comment.
yeah, that's another one!
| return true; | ||
| } | ||
|
|
||
| _index = _dictionary._count; |
|
|
||
| // If we're converting from properties, it's likely due to an 'add' to make sure we have at least | ||
| // the default amount of space. | ||
| capacity = Math.Max(DefaultCapacity, Math.Max(storage.Properties.Length, capacity)); |
There was a problem hiding this comment.
I found this issue while testing the new TryAdd method. We want to always have a minimum of 4. The properties -> array path could cause you to end up with an array of size 1, which would immediately get resized to 2, then next time you add... you get the idea.
|
Adding RVD.TryAdd
Notice that the new TryAdd is significantly faster than the old pattern. |
|
Use registers for RVD array accesses
|
|
Inlining for RVD TryGetValue
This actually makes our baseline much faster. |
| _acceptedValues.Add(key, value); | ||
| } | ||
| #endif | ||
|
|
Rewrite of TemplateBinder.GetValues
TreeRouter getting much faster with these changes, as I'm improving the shared stuff. |
|
@JamesNK - I plan to sort of keep iterating on this for the next day or so before merging any of this stuff. I'll probably fork the RVD changes into a separate PR since that's pretty isolated. |
rynowak
force-pushed
the
rynowak/linkgenerator-perf
branch
from
b308edc to
c08cee2
Compare
| metadata: new[] { new RouteValuesAddressMetadata(routeName: null, new RouteValueDictionary(new { controller = "Home", action = "In?dex", })) }); | ||
| "Home/Index/{id}", | ||
| defaults: new { controller = "Home", action = "Index", }, | ||
| metadata: new[] { new RouteValuesAddressMetadata(routeName: null, new RouteValueDictionary(new { controller = "Home", action = "Index", })) }); |
There was a problem hiding this comment.
These tests were invalid as-written. It should not be possible to have a route value appear as both a parameter and a required key.
|
@JamesNK - OK I think I've shaken out most of the obvious stuff here. Could you take another look? |
Porting changes from perf work in aspnet/Routing#788 Includes porting/adding the RVD benchmarks, as well as a new TryAdd method.
|
|
||
| var canCopyParameterAmbientValues = true; | ||
| // Make a new copy of the slots array, we'll use this as 'scratch' space. | ||
| var slots = new KeyValuePair<string, object>[_slots.Length]; |
There was a problem hiding this comment.
Add a comment that it will be used to back a RVD
There was a problem hiding this comment.
You can't stack this anyway since it contains reference types 😁
| // At this point we've captured all of the 'known' route values, but we have't | ||
| // handled an extra route values that were provided in 'values'. These all | ||
| // need to be included in the accepted values. | ||
| var acceptedValues = RouteValueDictionary.FromArray(slots); |
There was a problem hiding this comment.
Ah, I guess it can't be stackalloced since it is used to back the RVD
| } | ||
| } | ||
| [DebuggerDisplay("explicit null")] | ||
| private class SentinullValue |
There was a problem hiding this comment.
There are comments where this is used but it would be good to have a sentence explaining what this is for here
Porting changes from perf work in aspnet/Routing#788 Includes porting/adding the RVD benchmarks, as well as a new TryAdd method.
Porting changes from perf work in aspnet/Routing#788 Includes porting/adding the RVD benchmarks, as well as a new TryAdd method.
rynowak
force-pushed
the
rynowak/linkgenerator-perf
branch
from
0726ccd to
88455cf
Compare
2 participants
Will be sending batches of small-ish improvments to LinkGenerator perf here. I'll be using
LinkGenerationGithubBenchmark- this isn't a great benchmark in it's current state but it's useful enough as everything I'm doing so far is low-hanging fruit.Note that since this benchmark is using
TreeRouteras its baseline, anything we do that improves theTemplateBinderwill also improve theLinkGeneratoras this piece of infrastructure is shared.ORIGINAL
Add caching of TemplateBinder to LinkGenerator
This is an important step, because many of the rest of the optimizations we will make will rely on caching more data up front in
TemplateBinder. Note that the existing old routing code paths already cache template binder instances.Add a synthetic baseline
Adding a baseline based on RVD +
string.Format. This helps with context, because this is similar to what someone would do naively if they hand-rolled link generation. As you can see we're 6-10x slower (but with a lot more features). I doubt we'll beat this, but we should try to get within 2-3x.Move require keys to constructor
The change here just looks like noise, but since this data is totally static, this is a change I wanted to make anyway.
Inline RouteValueDictionary.Enumerator.MoveNext()
Before
After
ThrowHelper for RVD keys
There's a pretty good improvement getting/setting items directly. A lot of this seems like noise, but it's a good change to make.