Use a prefix tree as a backing store for ModelStateDictionary #4108

pranavkm · 2016-02-17T16:05:51Z

No description provided.

pranavkm · 2016-02-17T16:10:21Z

rynowak · 2016-02-17T18:02:34Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

@@ -20,7 +22,10 @@ public class ModelStateDictionary : IDictionary<string, ModelStateEntry>
        /// </summary>
        public static readonly int DefaultMaxAllowedErrors = 200;

-        private readonly Dictionary<string, ModelStateEntry> _innerDictionary;
+        private static readonly StringSegment EmptyStringSegment = new StringSegment(buffer: string.Empty);


StringSegment is a struct, there's really no benefit to caching this.

rynowak · 2016-02-17T18:19:05Z

⌚

davidfowl · 2016-02-19T09:52:02Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

+            var currentRoot = _root;
+            var previousIndex = 0;
+            int index;
+            while ((index = key.IndexOfAny(Delimiters, previousIndex)) != -1)


It would be great to show sample input in comments

pranavkm · 2016-02-19T16:48:23Z

Updated. Working on adding more tests

pranavkm · 2016-02-19T16:50:35Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

        }

        /// <inheritdoc />
-        public bool Contains(KeyValuePair<string, ModelStateEntry> item)
+        bool ICollection<KeyValuePair<string, ModelStateEntry>>.Contains(KeyValuePair<string, ModelStateEntry> item)


Not sure how to go about this - the equality comparison for item.Value will always return false since we create a copy of the one provided to us in Add or indexer. Should we just have this return false?

Same goes for Remove(KeyValuePair<string, ModelStateEntry>).

Yeah, I guess it's a bit of an issue that there's no deep equality for MSE.

IMO the better thing to do is throw an exception. (NotSupportedException)

pranavkm · 2016-02-19T17:35:48Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

+                throw new ArgumentOutOfRangeException(nameof(arrayIndex));
+            }
+
+            foreach (var item in this)


Could do this recursively to avoid the enumerator allocation. Not sure how much we care about CopyTo.

I'm not worried about it.

rynowak · 2016-02-19T17:48:11Z

⌚ for a few smaller comments

dougbu · 2016-03-11T03:09:21Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs

+                            searchKey.Offset,
+                            searchKey.Length,
+                            StringComparison.OrdinalIgnoreCase);
+                    }


In previous iteration, property "AB" would come before property "Z". Not sure how much difference the intrinsic order makes but why place shorter property names first?

Put another way, why not check the length only if string.Compare() returns 0?

It's going to be faster to compare lengths first than run through the string.

It's going to be faster to compare lengths first than run through the string.

Might only be visible in BadRequest(ModelState) responses and where actions use the enumerators. But the (early) optimization isn't worth this bizarre order.

We agreed offline to fix this up i.e. make the order sane. Correct?

I'm leaving this as is. The dictionary has no guarantees of ordering and I think it's fine the way we have things now. If we need ordering for BadRequest we should change it there.

dougbu · 2016-03-11T04:15:27Z

⌚ for a couple more doc comments. And perhaps Visit() could be a bit shorter.

dougbu · 2016-03-11T04:24:44Z

Larger question: With this change being allocation-neutral, is it worth going ahead? In particular is there a RPS improvement or does an execution hotspot go away?

If not, I'm a fan of the improved reliability of the ValidationSummary() error ordering. And the reduced public ModelStateDictionary surface might be worthwhile on its own (though a breaking change). But are these positive aspects enough to justify the extra complexity and code?

rynowak · 2016-03-14T15:12:35Z

Larger question: With this change being allocation-neutral, is it worth going ahead? In particular is there a RPS improvement or does an execution hotspot go away?

Yes. This change is focused on reducing the cost of things like GetValidationState which was implemented as a linear scan.

dougbu · 2016-03-14T15:50:48Z

the cost of things like GetValidationState which was implemented as a linear scan

If dotTrace should show a solid improvement, suggest measuring.

pranavkm · 2016-03-15T00:00:05Z

Sampling profiles of RouteMiddleware.Invoke before and after the change:

Before

ModelStateDictionary.GetFieldValidationState accounts for 141ms of a total of 1609ms (~10%) time taken for RouteMiddleware.Invoke.

After

ModelStateDictionary.GetFieldValidationState accounts for 31ms of a total of 3141ms (~1%) time taken for RouteMiddleware.Invoke.

pranavkm · 2016-03-15T02:04:45Z

Updated

dougbu · 2016-03-16T04:58:31Z

src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateEntry.cs

+
+using System.Collections.Generic;
+
+namespace Microsoft.AspNetCore.Mvc.ModelBinding


👏 for fixing the filename

dougbu · 2016-03-16T15:54:37Z

⌚ for #4108 (diff) resolution.

pranavkm · 2016-03-17T03:35:30Z

Updated.

dougbu · 2016-03-17T04:40:23Z

and please file the prefixed key follow-up issue.

dnfclas added the cla-already-signed label Feb 17, 2016

rynowak reviewed Feb 17, 2016
View reviewed changes

davidfowl reviewed Feb 19, 2016
View reviewed changes

pranavkm force-pushed the prkrishn/msd-prefix branch from 09ac7b4 to 11f7f26 Compare February 19, 2016 16:47

pranavkm reviewed Feb 19, 2016
View reviewed changes

pranavkm force-pushed the prkrishn/msd-prefix branch from 11f7f26 to b94dc3e Compare February 19, 2016 17:12

pranavkm reviewed Feb 19, 2016
View reviewed changes

pranavkm force-pushed the prkrishn/msd-prefix branch from b94dc3e to 936fce1 Compare February 19, 2016 18:34

dougbu reviewed Mar 11, 2016
View reviewed changes

pranavkm force-pushed the prkrishn/msd-prefix branch from 911a32c to fdc9b7f Compare March 15, 2016 02:04

pranavkm force-pushed the prkrishn/msd-prefix branch from fdc9b7f to 041ce28 Compare March 15, 2016 03:40

dougbu reviewed Mar 16, 2016
View reviewed changes

pranavkm added 2 commits March 16, 2016 19:29

Use a prefix tree as a backing store for ModelStateDictionary

89091bf

Changes per PR comments

52625ba

pranavkm force-pushed the prkrishn/msd-prefix branch from 041ce28 to 52625ba Compare March 17, 2016 03:33

pranavkm closed this Mar 17, 2016

pranavkm deleted the prkrishn/msd-prefix branch March 17, 2016 17:52

pranavkm mentioned this pull request Mar 17, 2016

ValidationHelpers for prefixed entries does not use display order #4308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a prefix tree as a backing store for ModelStateDictionary #4108

Use a prefix tree as a backing store for ModelStateDictionary #4108

pranavkm commented Feb 17, 2016

pranavkm commented Feb 17, 2016

rynowak Feb 17, 2016

rynowak commented Feb 17, 2016

davidfowl Feb 19, 2016

pranavkm commented Feb 19, 2016

pranavkm Feb 19, 2016

pranavkm Feb 19, 2016

rynowak Feb 19, 2016

pranavkm Feb 19, 2016

pranavkm Feb 19, 2016

rynowak Feb 19, 2016

rynowak commented Feb 19, 2016

dougbu Mar 11, 2016

dougbu Mar 11, 2016

pranavkm Mar 11, 2016

dougbu Mar 13, 2016

dougbu Mar 16, 2016

pranavkm Mar 16, 2016

dougbu commented Mar 11, 2016

dougbu commented Mar 11, 2016

rynowak commented Mar 14, 2016

dougbu commented Mar 14, 2016

pranavkm commented Mar 15, 2016

pranavkm commented Mar 15, 2016

dougbu Mar 16, 2016

dougbu commented Mar 16, 2016

pranavkm commented Mar 17, 2016

dougbu commented Mar 17, 2016


		using System.Collections.Generic;

		namespace Microsoft.AspNetCore.Mvc.ModelBinding

Use a prefix tree as a backing store for ModelStateDictionary #4108

Use a prefix tree as a backing store for ModelStateDictionary #4108

Conversation

pranavkm commented Feb 17, 2016

pranavkm commented Feb 17, 2016

Choose a reason for hiding this comment

rynowak commented Feb 17, 2016

Choose a reason for hiding this comment

pranavkm commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Mar 11, 2016

dougbu commented Mar 11, 2016

rynowak commented Mar 14, 2016

dougbu commented Mar 14, 2016

pranavkm commented Mar 15, 2016

Before

After

pranavkm commented Mar 15, 2016

Choose a reason for hiding this comment

dougbu commented Mar 16, 2016

pranavkm commented Mar 17, 2016

dougbu commented Mar 17, 2016