Skip to content

Issue/8284#8289

Merged
sebastienros merged 1 commit into
OrchardCMS:1.10.xfrom
LaserSrl:issue/8284
Dec 5, 2019
Merged

Issue/8284#8289
sebastienros merged 1 commit into
OrchardCMS:1.10.xfrom
LaserSrl:issue/8284

Conversation

@MatteoPiovanelli

Copy link
Copy Markdown
Contributor

Fixes #8284

Tested locally Starting off Orchard 1.10.x as well as on our custom configuration.

The changes to NavigationManager improve performances on menu-heavy frontends, as well as a little bit on the backoffice (because they also cache some of the operations that are done for the admin menu there)

The other changes can really only be measured when serving cached content, as those operations are not that relevant compared to those required to generate frontend views. When serving stuff out of cache however those changes combined gave around 30% quicker responses on my a couple different test configurations.

@sebastienros

Copy link
Copy Markdown
Member

LGTM

@BenedekFarkas are you ok with it? If you don't answer I will merge myself.

@MatteoPiovanelli

Copy link
Copy Markdown
Contributor Author

I was thinking about this PR earlier under the shower and I think I am not correctly handling the dashboard/backoffice menu. I should add an evict on enable/disable of a feature, but I am not sure that would be enough.
Better to not cache the backoffice menu, by adding an AdminFilter check

@MatteoPiovanelli

Copy link
Copy Markdown
Contributor Author

Regarding admin side menus:
I still don't feel like it's trivial to cover all conditions, but:

  • On changing the Listable property of a ContentType, the menu gets updated correctly.
  • On Creating a new ContentType, the menu gets updated correctly
  • (basically, whenever we post for the creation or edit of a ContentType, as far as I can tell, that menu cache gets evicted correctly)
  • When I Enable or Disable a feature, the cache gets evicted, regardless of the fact that the feature may or may not change the menu.
    That's all "out of the box", without having to explicitly write anything.

However, AdminMenuPart does not cause an update of that value. That means there is at least this one case that is not handled. I have to assume there will be more.
Hence I updated the PR to prevent caching of admin menus.

@strongus

Copy link
Copy Markdown

I merged this fix into my project it dramatically improved the speed of the Admin side of the site. Thanks so much for this! The slowness was driving me nuts.

@MatteoPiovanelli

Copy link
Copy Markdown
Contributor Author

I found some issues with the approach we took for caching frontend menu items.

In summary:
GetSources(string) and GetSource(IContent):
The results of these two methods depend on their input parameters as well as a bunch of injected IDependencies. Even though we are sure that the cache would be cleared if the IEnumerable of the IDependencies changed, we cannot be sure that none of those depends on settings or properties that would not reset/evict the cache upon changing. As a consequence, we cannot simply and safely cache the results of these methods. That would mean that a cache for a menu would have to monitor all possible changes to all its items. Since there is no easy way to do that, we decided to rollback the changes we were proposing here for Navigation.

The other changes (caching of SslSettingPart and SiteSelector) still stand.

}

private void Invalidate(SslSettingsPart content) {
_signals.Trigger($"SslSettingsPart_{content.Id}");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep it but I think it's not necessary. And I don't see it used anywhere else either.

@sebastienros
sebastienros merged commit abfe7a0 into OrchardCMS:1.10.x Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants