[Perf] ViewDataDictionary is copied and resized many times

[NickCraver]

When we spin up a child view, we're seeing many copies of the ViewData dictionary. Currently instead of 1 copy for the child view (we don't even want in many cases, but given where Model lives, that's a non-trivial change for the <T>), we see 5 copies in total. This is in reference to Issue #870.

4 of the 5 come from creations of AjaxHelper<T> and HtmlHelper<T> both of which have a ViewDataDictionary<T> constructor called internally.

In the non-generic WebViewPage there are 2 copies made:

public virtual void InitHelpers()
{
    Ajax = new AjaxHelper<object>(ViewContext, this);
    Html = new HtmlHelper<object>(ViewContext, this);
    Url = new UrlHelper(ViewContext.RequestContext);
}

And in WebViewPage<T> there are 2 additional copies in the InitHelpers() override:

public override void InitHelpers()
{
    base.InitHelpers();

    Ajax = new AjaxHelper<TModel>(ViewContext, this);
    Html = new HtmlHelper<TModel>(ViewContext, this);
}

Also in WebViewPage<T> in the SetViewData() override is another copy:

protected override void SetViewData(ViewDataDictionary viewData)
{
    _viewData = new ViewDataDictionary<TModel>(viewData);

    base.SetViewData(_viewData);
}

Since ViewDataDictionary<T> performs a shallow copy in its base ViewDataDictionary constructor here:

foreach (var entry in dictionary)
{
    _innerDictionary.Add(entry.Key, entry.Value);
}

We get a shallow copy of the dictionary. This has 2 issues for us:

1. We get a lot of duplicated shallow copies that aren't really needed, they're only there for the T Model when it comes down to it, times 4 here.
2. More importantly, the Dictionary<string, object> is resized every time it's actually used, this is because it's initialized at a capacity of 0:

private readonly Dictionary<string, object> _innerDictionary = 
     new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);

Number 2 means even for a few values (we typically have 2-4) we are eating a dictionary .Resize() 5 or more times, per view. We have hundreds of views or more on many pages so this stings pretty hard.

The way I see it, the best solution is probably to create the Dictionary<string, object> differently: in the pipeline the constructor would create a properly sized one via the new Dictionary<string, object>(IDictionary<string, object> dictionary) constructor, instead of inflating one from 0 size. The new hiding on the Ajax and Html "overrides" in WebViewPage<T> over WebViewPage create an additional 2 copies...can these not simply reference the ViewDataDictionary<T> from the WebViewPage rather than copying their own? I can't find a case where these should actually differ but I may certainly be missing something.

Assuming they still need their own shallow copy, the top of ViewDataDictionary could still look like this:

public class ViewDataDictionary : IDictionary<string, object>
{
    private readonly Dictionary<string, object> _innerDictionary;

    public ViewDataDictionary() : this((object)null) { }

    public ViewDataDictionary(object model)
    {
        Model = model; 
        _innerDictionary = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
    }

    public ViewDataDictionary(ViewDataDictionary dictionary)
    {
        if (dictionary == null)
        {
            throw new ArgumentNullException("dictionary");
        }

        if (dictionary.Count > 0)
        {
            _innerDictionary = new Dictionary<string, object>(dictionary);
        }
        foreach (var entry in dictionary.ModelState)
        {
            ModelState.Add(entry.Key, entry.Value);
        }

        Model = dictionary.Model;
        TemplateInfo = dictionary.TemplateInfo;

        // PERF: Don't unnecessarily instantiate the model metadata
        _modelMetadata = dictionary._modelMetadata;
    }

This takes care of the Resize() pain aspect in a fairly concise way, but not the 5x work and allocation issue. I'd propose that HtmlHelper and AjaxHelper simply set the reference if the TModel matches already, something like this:

public HtmlHelper(ViewContext viewContext, IViewDataContainer viewDataContainer, RouteCollection routeCollection)
    : base(viewContext, viewDataContainer, routeCollection)
{
    var sameParentViewData = viewDataContainer.ViewData as ViewDataDictionary<TModel>;
    if (sameParentViewData != null)
    {
        _viewData = sameParentViewData;
    }
    else
    {
        _viewData = new ViewDataDictionary<TModel>(viewDataContainer.ViewData);
    }
}

Doing this in both AjaxHelper and HtmlHelper would again be fairly concise changes with hopefully no negative impact but still have a huge impact on allocations at our case.

[dougbu]

@NickCraver please note your last suggestion would undermine the contract MVC upholds w.r.t. ViewDataDictionary instances -- individual settings are scoped, not global. For example, a change to ViewBag.MyItem in a ViewComponent does not affect reads of ViewBag.MyItem (or equivalently ViewData["MyItem"]) in the containing IView.

Therefore the solution to your last issue must involve copy-on-write semantics. This would be relatively straightforward since ViewDataDictionary does not directly expose _data (_innerDictionary in MVC 5.2) to callers. That is, we have complete control of how the indexer's setter, Add(), Clear(), and Remove() are implemented.

[NickCraver]

@dougbu ah true - this isn't a concern for us at all since the instances are 1:1 with their usages here, but I see how that's a definite blocker as-is. It would be good if we could at least narrow them quite a bit as well...not even get to this step.

The multiple instances on the base could be eliminated just by not instantiating the hidden implementations in the WebViewPage base. By making the WebViewPage<T>.InitHelpers() method not call the base at all. Since it's 1 layer up, do you see any issue in simply calling the only allocation that's desirable, for UrlHelper?

I'm proposing changing this:

public override void InitHelpers()
{
    base.InitHelpers();

    Ajax = new AjaxHelper<TModel>(ViewContext, this);
    Html = new HtmlHelper<TModel>(ViewContext, this);
}

To this:

public override void InitHelpers()
{
    Url = new UrlHelper(ViewContext.RequestContext);
    Ajax = new AjaxHelper<TModel>(ViewContext, this);
    Html = new HtmlHelper<TModel>(ViewContext, this);
}

This takes us down from 5 to 3 copies with minimal changes. While the write-on-change semantics are the way to narrow it down as much as possible while keeping the contract, this would be a quick win in performance. Since they're new hidden and not meant to be accessed in anyway, it seems wasteful for them to get created. Thoughts?

[yishaigalatzer]

@NickCraver bug got assigned to @pranavkm, we will get a PR out in the next couple of weeks.

[yishaigalatzer]

We are going to also look at current MVC after fixing this issue - https://aspnetwebstack.codeplex.com/workitem/2085

[pranavkm]

There's quite a few changes in the execution model in Mvc 6, which makes some of the analysis here not particularly applicable.

1. In MVC 6, helpers no longer make copies of ViewDataDictionary (VDD) when they are initialized \ contextualized. This removes the 3 VDD creations that happen as part of InitHelpers. I started a thread here Helpers do not create copies of ViewDataDictionary when contextualized #954 to ensure that this behavior change is correct.

2. In the lifetime of a view, we create

   a. One VDD for the Controller (if it has a injected ViewDataDictionary property),
   b. One for each _ViewStart. (This might a bug due to the way _ViewStarts are generated. Today _ViewStarts are generated as RazorPage which causes us to create a VDD wrapping the controller's VDD. Tracked via _ViewStart generation should be different from regular view pages. #952)
   c. One for the page
   d. One for each partial \ view component that is encountered as part of the page lifecycle.
   e. One for each Layout
   For a minimal View (one _ViewStart, no partials, one Layout), the number of VDDs created is 4.

At this stage, something along the lines of the copy-On-write might be the most impactful change.

[pranavkm]

We decided to leave the behavior discussed in #954 until we can see a strong reason to change it to the Mvc 5 behavior. That said, it would be incorrect to port this behavior back to Mvc 5.

The plan would be to to investigate the impact suggested in #878 (comment) in Mvc 5 in addition to Copy-On-Write that we have in Mvc 6. The bug on CodePlex (https://aspnetwebstack.codeplex.com/workitem/2085) links back to this location so we can make use of the discussion available here.

[pranavkm]

I ported over the CopyOnWriteDictionary changes to Mvc 5 and additionally was able to defer the creation of the two helpers on the base type that are discussed in #878 (comment).

[NickCraver]

Thanks guys, looking forward to testing this soon as I'm able!

[yishaigalatzer]

@NickCraver what a great suggestion. The perf test was very happy with you :)