Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance issue with GlobalSection viewlet #3035

Closed
thet opened this issue Feb 7, 2020 · 3 comments · Fixed by #3329
Closed

Performance issue with GlobalSection viewlet #3035

thet opened this issue Feb 7, 2020 · 3 comments · Fixed by #3329

Comments

@thet
Copy link
Member

thet commented Feb 7, 2020

The PR plone/plone.app.layout#192 restored the ability to show excluded items in the global section viewlet when any of the current path's parent's have exclude_from_navigation set to True.

The PR is implemented in a way that the exclude_from_navigation catalog index (which was created for the PLIP #2516 ) is used when show_excluded_items is set to False. This is good.
But show_excluded_items is set per default to True:

This is only an issue when the navigation depth is set to something higher than 1, so when a navigation tree is rendered with subnavigation. Plone's default is 3.

For a quick test I got these results:

Navigation always rendered as a tile - @@plone.app.standardtiles.navigation

navigation_depth=1navigation tree with total 9 items:
show_excluded_items=True: 50-90ms.
show_excluded_items=False: 50-90ms.

navigation_depth=2navigation tree with total 40 items:
show_excluded_items=True: 160ms to 230ms.
show_excluded_items=False: 60-90ms.

navigation_depth=3navigation tree with total 67 items:
show_excluded_items=True: 300ms to 600ms.
show_excluded_items=False: 80-120ms.

/cc @petschki @agitator @jensens @sneridagh @ale-rt

@thet
Copy link
Member Author

thet commented Feb 26, 2020

Performance fix:
in registry.xml:

<?xml version="1.0"?>
<registry>
  <records prefix="plone" interface="Products.CMFPlone.interfaces.controlpanel.INavigationSchema">
    <value key="show_excluded_items">False</value>
  </records>
</registry>

@agitator
Copy link
Member

I'd say this option should be False by default

@jensens
Copy link
Sponsor Member

jensens commented Feb 26, 2020

I think for Plone 6 we are save to just change the default. No migration step needed, since keeping the behavior from before upgrade is more important.

thet added a commit that referenced this issue Sep 30, 2021
…default.

Setting it to ``True`` can introduce a performance problem.
``False`` should be the default, also from user expectation for the ``exclude_from_nav`` setting on conte
nt items.
No upgrade step!
Previous behavior is just kept, unless you override it manually.
See: #3055, first comment.
Use this registry snippet to set it false:
```
<?xml version="1.0"?>
<registry>
  <records prefix="plone" interface="Products.CMFPlone.interfaces.controlpanel.INavigationSchema">
    <value key="show_excluded_items">False</value>
  </records>
</registry>
```
Fixes: #3035
thet added a commit that referenced this issue Oct 13, 2021
…default.

Setting it to ``True`` can introduce a performance problem.
``False`` should be the default, also from user expectation for the ``exclude_from_nav`` setting on conte
nt items.
No upgrade step!
Previous behavior is just kept, unless you override it manually.
See: #3055, first comment.
Use this registry snippet to set it false:
```
<?xml version="1.0"?>
<registry>
  <records prefix="plone" interface="Products.CMFPlone.interfaces.controlpanel.INavigationSchema">
    <value key="show_excluded_items">False</value>
  </records>
</registry>
```
Fixes: #3035
thet added a commit that referenced this issue Oct 13, 2021
…default.

Setting it to ``True`` can introduce a performance problem.
``False`` should be the default, also from user expectation for the ``exclude_from_nav`` setting on conte
nt items.
No upgrade step!
Previous behavior is just kept, unless you override it manually.
See: #3055, first comment.
Use this registry snippet to set it false:
```
<?xml version="1.0"?>
<registry>
  <records prefix="plone" interface="Products.CMFPlone.interfaces.controlpanel.INavigationSchema">
    <value key="show_excluded_items">False</value>
  </records>
</registry>
```
Fixes: #3035

Co-authored-by: Peter Mathis <peter.mathis@kombinat.at>
thet added a commit that referenced this issue Oct 13, 2021
…default.

Setting it to ``True`` can introduce a performance problem.
``False`` should be the default, also from user expectation for the ``exclude_from_nav`` setting on conte
nt items.
No upgrade step!
Previous behavior is just kept, unless you override it manually.
See: #3055, first comment.
Use this registry snippet to set it false:
```
<?xml version="1.0"?>
<registry>
  <records prefix="plone" interface="Products.CMFPlone.interfaces.controlpanel.INavigationSchema">
    <value key="show_excluded_items">False</value>
  </records>
</registry>
```
Fixes: #3035

Co-authored-by: Peter Mathis <peter.mathis@kombinat.at>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants