Performance issue when using ContentModel or ModelsBuilder model action parameter in a RenderController.

[Adolfi]

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

9.5.1

Bug summary

When using a RenderController for Route hijacking, if you use the action with ContentModel or ModelsBuilder model parameter you will experience massive performance problems when adding more and more content and properties in the CMS.

This is all the code I've written for these tests:

public class PageController : RenderController
{
    public PageController(ILogger<RenderController> logger, ICompositeViewEngine compositeViewEngine, IUmbracoContextAccessor umbracoContextAccessor) : base(logger, compositeViewEngine, umbracoContextAccessor) { }
 
    // Performance is not affected using this action, non-parameter and CurrentPage.
    public IActionResult PageUsingCurrentPage()
    {
        return View(new PageViewModel(CurrentPage));
    }

    // Performance is heavily affected when using this action with ContentModel parameter.
    public IActionResult PageUsingContentModel(ContentModel model)
    {
        return View(new PageViewModel(model.Content));
    }
    
    // Performance is heavily affected when using this action with ModelsBuilder generated parameter.
    public IActionResult PageUsingModelsBuilder(Page model)
    {
        return View(new PageViewModel(model));
    }
}

public class PageViewModel : ContentModel
{
    public PageViewModel(IPublishedContent content) : base(content)
    {
        this.Name = content.Name;
    }

    public string Name { get; set; }
}

Specifics

Here are my tests when adding more and more content nodes and properties when using the ContentModel and ModelsBuilder parameter. The tests compare using the ContentModel/ModelsBuilder parameter to using the non-parameter with the CurrentPage property for reference. As you can see the load time is not affected when using CurrentPage but it is very much affected when using ContentModel/ModelsBuilder.

• 5 pages (not really any noticeable difference)
• 20 pages (no noticeable difference, maybe a little but could be luck)
• 300 pages (starting to notice real difference): CurrentPage: 4-6ms, ContentModel/ModelsBuilder: 26-33ms
• 300 pages and adding 2 properties to the doctype (noticeable change): CurrentPage: 4-6ms, ContentModel/ModelsBuilder: 48-77ms
• 300 pages and adding 5 more properties: CurrentPage: 4-6ms, ContentModel/ModelsBuilder: 100-200ms
• 500 pages: CurrentPage: 4-6ms, ContentModel/ModelsBuilder: 500-700ms
• 500 pages and adding 5 more properties: CurrentPage: 4-6ms, ContentModel/ModelsBuilder: 1.1-1.3 seconds!

This is as far as my tests goes but we first noticed this on one of our production sites that started closing in on 1000+ nodes and the load time was close to 7 seconds. Switching to CurrentPage instead of ContentModel decreased the load time to 50ms.

Steps to reproduce

1. Install empty Umbraco project.
2. Create a doctype (allow at root, allow children of same type).
3. Create 3 templates that you later will map against your controller actions (in my example PageUsingCurrentPage, PageUsingContentModel and PageUsingModelsBuilder).
4. Create a root node in the content tree and create a few children nodes.
5. Create a RenderController for the new doctype.
6. Add Action methods using the ContentModel model parameter, another using ModelsBuilder generated model and another one using CurrentPage. Make sure all actions return the same view to rule out and view-specific differences in load time.. (see example code).
7. Browse any of the created pages with ?umbDebug=true to measure load time and switch between the 3 difference templates (to compare CurrentPage and ModelsBuilder and ContentModel performance).

Expected result / actual result

Add more and more content nodes and then add more properties to this doctype and monitor the load time when browsing the page. The more nodes/properties you add the higher the load time gets and you will start to notice massive performance issues at 500+ nodes and 10 properties. (see test results the specifics section)

Notice that these tests are on my local machine and is seems like the more resources you have the shorter the load time gets for ContentModel/ModelsBuilder because we could decrease the load time on our production site by assigning more resources in Azure to a higher monthly cost. However they never come close to as fast with using CurrentPage.

[dawoe]

Haven't done any digging yet. But this could be a place to start investigating.

https://github.com/umbraco/Umbraco-CMS/blob/9326cc5fc64a85e8e7fc19ae7faa726e33c33480/src/Umbraco.Web.Common/ModelBinders/ContentModelBinder.cs#L27

[DanDiplo]

Is anyone from the core team taking this seriously as it's still a problem in Umbraco 10.0.1 ? I can see a massive performance issue when using a model in the action. This was never an issue in Umbraco 8.

[jawood1]

This is a major issue and I can confirm still happening on 10.1.0.

I have a page using the ContentModel that is taking 50 seconds to load the page as the site has thousands of nodes.

The site in question was upgraded from v8 to v10.

As suggested a work around is to use CurrentPage if possible.

When running the tool Prefix I'm getting this in the trace.

[nul800sebastiaan]

Thanks for the reports, we aim to investigate this in our next sprint (starting Sept 12).

[kjac]

@Adolfi thank you so much for this awesomely detailed issue report 💪 🚀

I can reproduce it on latest V10 - I'll start digging into it.

[kjac]

@Adolfi @DanDiplo @jawood1 this is tied to how ASP.NET Core performs validation; specifically validation of object graphs. There is a fix for it, but as it stands we likely can't ship it until V11, as it is a behavioural breaking change.

However I will encourage you to try it out on your sites, as it works just fine with V10. It goes as follows:

1. Add the following classes to your project:

public class CustomMvcConfigureOptions : IConfigureOptions<MvcOptions>
{
    public void Configure(MvcOptions options)
    {
        options.ModelValidatorProviders.Insert(0, new BypassRenderingModelValidatorProvider());
        options.ModelMetadataDetailsProviders.Add(new BypassRenderingModelValidationMetadataProvider());
    }
}

public class BypassRenderingModelValidationMetadataProvider : IValidationMetadataProvider
{
    public void CreateValidationMetadata(ValidationMetadataProviderContext context)
    {
        if (typeof(ContentModel).IsAssignableFrom(context.Key.ModelType)
            || typeof(IPublishedContent).IsAssignableFrom(context.Key.ModelType))
        {
            context.ValidationMetadata.ValidateChildren = false;
        }
    }
}

public class BypassRenderingModelValidatorProvider : IModelValidatorProvider
{
    public void CreateValidators(ModelValidatorProviderContext context)
    {
        if (typeof(ContentModel).IsAssignableFrom(context.ModelMetadata.ModelType)
            || typeof(IPublishedContent).IsAssignableFrom(context.ModelMetadata.ModelType))
        {
            context.Results.Add(new ValidatorItem
            {
                Validator = new BypassRenderingModelValidator(),
                IsReusable = true
            });
        }
    }
}

public class BypassRenderingModelValidator : IModelValidator
{
    public IEnumerable<ModelValidationResult> Validate(ModelValidationContext context)
        => Array.Empty<ModelValidationResult>();
}

2. Register the custom MVC options in Startup:

public void ConfigureServices(IServiceCollection services)
{
	// ...
	services.ConfigureOptions<CustomMvcConfigureOptions>();
	// ...
}

[darrenm-dwl]

@kjac Thanks for the workaround, it works like a charm and the issue has been plaguing me!

[DanDiplo]

Great detective work, @kjac Thanks for this! If it is a breaking change, I'm wondering if you could add the CustomMvcConfigureOptions class (and associated classes) to the core and then then just comment it out in the Startup class, so people can easily enable, if needed?

[kjac]

@DanDiplo I see your point, but the behavior it is (theoretically) breaking is pretty broken at the moment, as this issue clearly shows. We want this to "just work" for people, not be something you have to know to opt into. If by chance the new behavior ends up breaking something in edge case scenarios, we'd rather help writing a new CustomMvcConfigureOptions that re-enables the object graph validation.

[DanDiplo]

@kjac Sorry, I meant comment-out for v10. Definitely add it to 11. I would be happy for it to be added in 10, too, but HQ call :)

[kjac]

@DanDiplo Aah I see where you're going. I would be somewhat hesitant to add this workaround to 10.3, since it's right around the corner and will be the last 10 minor. The fix will be included in 11, so adding the workaround to 10.3 would mean adding something to core that would be marked as obsolete from the very beginning. That doesn't really sit well with me 😄

[Adolfi]

Wow thanks @kjac that is awesome!

I just tried this out and I can verify that:

1. This is still an issue in Umbraco 10.2.0
2. Adding your commented classes solves the issue! Load time is same as when using CurrentPage when using ContentModels and the load time is not affected when adding more pages/properties to the backoffice.

#h5yr @kjac

[kjac]

Thanks for testing it @Adolfi 💪 glad it worked out well.

The PR #12999 that fixes this for V11 has just been merged, so I'm closing this issue now. Feel free to reopen it if you think it should remain open.

[emmagarland]

Good find @kjac.

Just a headsup (nearly a year later!) that since we are building to LTS, we have this issue in Umbraco version 10.6.1, where search/listing pages using the RenderController and passing in a ContentModel took near 9 seconds to load. I was looking everywhere to work out what was going on.

Luckily as soon as we found this post and put @kjac's code change in to stop the model validation, they were super fast again.

I gather that technically it is a breaking change and hence was not cherry-picked into v10 (plus it will eventually become obsolete). However, since 10 is still LTS for several months, this could mean many Umbraco v10 sites are slowing down with this issue and people will not know why unless they happen across this post.

[karltynan]

I agree with @emmagarland... I was luck enough to find this GitHub issue and fix (which absolutely works a treat #h5yr), but others may not be so lucky. Definitely something I think should be added to V10 LTS even though it will be instantly obsolete.

[protherj]

So glad I found this. We were having this issue on an upgrade and this worked perfectly!

[kjac]

Hi everyone 👋 thank you for your feedback here. It's very appreciated. Seems this issue is less of an edge case after all.

I do sympathise with the point of having this in V10. And V10 isn't "instantly obsolete", the LTS security phase ends mid 2025 if I'm not mistaken 😄

It is however still a breaking change. I'll take it to the team next week and discuss options.

For good measure I need to point out that this is a workaround for controllers not following the standards introduced in V9. If you use CurrentPage instead of injecting models into the controller actions, things should run equally fast as with this workaround applied.

As input to me raising this with the team, are there anyone seeing obstacles in using CurrentPage, other than the time spent rewriting controllers? I'm fully aware of the (lack of) discoverability when upgrading and facing this issue... what I'm looking for is arguments why we absolutely need this om V10 - i.e. stuff that worked in the V7/V8 ways of writing controllers, but will not work with CurrentPage.

[Adolfi]

Hi Kenn. Yes the new v9 standard/documentation changed from previously recommending injecting ContentModel in controller action to instead recommend to use CurrentPage in the new docs.

Maybe the new (and possibly also old docs for heads up) documentation need some sort of big red warning saying something like "Althougt it is still possible to inject ContentModel in your controller actions (for backward compability) this might case performance issues if you do not follow the instruction here (link to this conversation or my blogpost).

However this is most likey an issue for old-dogs like myself that are following the old standard and when these performance issues arised it was not super obvious to assume that it was due to injection of models or even my controllers so it is a bit hard to know what to search for. New Umbracians (joining after v9) are most likely not facing these issues since they are probably following the new v9 standard.

Regarding your question on obstacles in using CurrentPage: Historically, I've always avoided the static CurrentPage like the plauge, mainly for testability reasons. It's a lot easier to just new up a ContentModel object and pass in to an action I want to unit test instead of having to fiddle with context routing and whatever else magic needed to have a fully working CurrentPage. Im not saying it's impossible and probalby easier to mock CurrentPage today that back in the days, but I guess it is not as easy as injecting a ContentModel?

Another alternative is, now when UmbracoHelper is so much easier to mock, is to use UmbracoHelper.AssignedContentItem instead of CurrentPage in your controllers and mocks. So much easier to assign.

Other than testability I see no problem switching to CurrentPage instead of ContentModel when facing this issue. It took us probably 15 minutes so switch everywhere and this huge performance issues was resolved. The problem I guess is making people awear of this issue beacuse it is not obvious to troubleshoot, which seems to be confirmed by a lot of the comments in this thread.

Thanks for addressing this Kenn and for everyone chipping in!

[karltynan]

Hi Kenn,

"Obsolete" was regarding your comment about the code, not the version:

...adding the workaround to 10.3 would mean adding something to core that would be marked as obsolete from the very beginning.

In terms of injecting models, this is usually for testing, making it a lot easier to test our controller and its logic.

Many thanks!

[emmagarland]

Interesting feedback to read from everyone.

I've tried a very quick experiment taking out the hotfix, and as expected, ContentModel and IPublishedContent seem just as slow as each other. CurrentPage is fast but not overly testable.

• I firstly tried to change the code to inject the UmbracoHelper via the injected IUmbracoHelperAccessor, as per the docs, to set this up in the unit tests to return a mocked IPublishedContent as the AssignedContentItem. However, I got some issues that I had previously when trying to inject this (might be the way I was doing it?)
• In the end, I used the IUmbracoContextAccessor to get the published content from umbracoContext!.PublishedRequest!.PublishedContent. I setup the tests to return the mock page from this and it worked (although more lines of setup were required than plain ContentModel). This was fast again.

I agree that a warning on the v10 docs is a good idea. It will also then be indexed for when people are searching for performance issues on Umbraco 10.

I think it is also a likely scenario if someone is upgrading a v8 site for example, that they will be using Index(ContentModel model, since it still works that way (even if CurrentPage is recommended in the later docs).

[kjac]

Thanks for the inputs everyone ❤️ it's always good to get some perspectives on these things.

[DanDiplo]

@kjac you say it's a breaking change, but in practical terms what would it break? I can't really understand what situation adding this would break anything. Just curious....