Modifying ViewDataDictionary and ModelStateDictionary to copy on write instead of eagerly copying. #973
Conversation
instead of eagerly copying. Partial fix for #878
pranavkm
changed the title
| return ReadDictionary.Contains(item); | ||
| } | ||
|
|
||
| public virtual void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex) |
There was a problem hiding this comment.
This might not work if TKey is a value type though. I'd rather have the calling code do the NotNull checks for us
There was a problem hiding this comment.
We could add a a type constraint to TKey since the 2 consumers we have now have string keys, but seeing that it's not public API, seems crummy. Thoughts?
There was a problem hiding this comment.
its not an error to check a struct for nullity it's a warning. that concern would come up elsewhere when implementing [NotNull]
There was a problem hiding this comment.
You'd still fail with warning as errors. Regardless, @dougbu suggested changing this to be an IDictionary(string, TValue) since either consumer is of that type. That should avoid this issue
|
other than the comment on removing the copying of the modelstate dictionary, I'm fine with most of the rest. Let's talk about that one in person. Other than that |
when the other folks say 
|
Updated PR |
| _data.Add(entry.Key, entry.Value); | ||
| } | ||
|
|
||
| TemplateInfo = new TemplateInfo(source.TemplateInfo); |
There was a problem hiding this comment.
pls find a way to avoid constructing 2 new TemplateInfo instances when this constructor is called. TemplateInfo will already be non-null when this line is reached.
|
⌚ waiting, primarily for tests that |
|
Updated |
| @@ -13,7 +13,7 @@ namespace Microsoft.AspNet.Mvc | |||
| { | |||
| public class ViewDataDictionary : IDictionary<string, object> | |||
| { | |||
| private readonly IDictionary<string, object> _data; | |||
| internal readonly IDictionary<string, object> _data; | |||
There was a problem hiding this comment.
Ick. Just internal but ick all the same. How about an internal property exposing _data's Type? Mark that with // for testing only).
Same for the newly-internal field in ModelStateDictionary.
|
🚢 it if @rynowak agrees his comments have been addressed and once you do something about those |
|
|
Partial fix for #878