verbb / navigation

A Craft CMS plugin to create navigation menus for your site.
Other
90 stars 23 forks source link

Lots of duplicated queries #319

Closed proimage closed 2 years ago

proimage commented 2 years ago

Describe the bug

I was checking the Craft developer toolbar, and noticed it was reporting that there were 59 duplicated queries out of 268 total on a certain page. When I sorted the table of queries by the Duplicated column in descending order, I saw this:

image

No idea if this is expected, but it seems like it could be optimized. I certainly don't think I have 18 replicated calls to the same Navigation function in the templates that comprise this page.

Steps to reproduce

  1. Not entirely sure, but if the solution isn't obvious from the screenshot, I can look into a reduced use-case. :)
  2. Profit!

Craft CMS version

Craft CMS 3.7.55.1

Plugin version

1.4.26

Multi-site?

Yes

Additional context

Again, if what the screenshot shows doesn't make the solution obvious, I'll dig deeper. :)

engram-design commented 2 years ago

So I'm not seeing any duplication of that level. How many times are you outputting the navigation on your page?

image

How many calls to craft.navigation.nodes() (or similar) do you have in a page load? From this image, it looks like 18. As some background, this call is done every time a NodeQuery is created, to check against the currently installed version of Navigation. as over the years we've added different query params, so this is to protect against that.

It should directly correspond to the number of times you output your navigation. And it's also only there for Craft 3, where this is removed in Craft 4.

proimage commented 2 years ago

I certainly don't think we're calling it 18 times, but perhaps we're using a macro wrong? Let's see...

Main Nav

The main nav menu template (which is called only once obv.) has this code:

{% import '_includes/macros' as render %}

{% set nodes = craft.navigation.nodes('mainNavigation').level(1).all() %}
{% for node in nodes %}
    {{ render.navNode(node) }}
{% endfor %}

The _includes/macros template has this bit for brevity:

{% macro navNode(node) %}
    {% include "_includes/macros/navNode" %}
{% endmacro %}

and then _includes/macros/navNode just uses the data in the node variable passed to it (including a {% for subnode in node.children.all() %} loop) to output the HTML of the menu & submenus.

Mobile Donation Button

On mobile screens, there's a CTA donation button fixed to the bottom of the screen. It has a small submenu of donation options, and is a specific node of the main menu, designated by a lightwitch: image

{# GIVE BUTTON #}
{# Extracting Give sub menu from main navigation #}
{% cache globally %}
    {% set nodes = craft.navigation.nodes({ handle: 'mainNavigation' }).level(1).all() %}
    {% for node in nodes %}
        {# Check the lightswitch state #}
        {% if node.isGiveMenuRoot %}
        {# Output donation button here & check for subnodes #}
            {% for subnode in node.children.all() %}
                {# Output subnode code here #}
            {% endfor %}
        {% endif %}
    {% endfor %}
{% endcache %}

Footer "Sitemap"

Finally, the footer has the typical "sitemap" of various links that aren't prominent enough to go in the main menu.

footer.twig:

{% import '_includes/macros' as render %}

{% cache globally %}
    {{ render.generateNavigation('sitemap') }}
{% endcache %}

Then in _includes/macros.twig again:

{% macro generateNavigation(navHandle, options) %}
    {% set nodes = craft.navigation.nodes().handle(navHandle).all() %}

    {% nav node in nodes %}
        <li>
            <a href="{{ node.url }}">
                {{ node.title }}
            </a>

            {% ifchildren %}
                <ul>
                    {% children %}
                </ul>
            {% endifchildren %}
        </li>
    {% endnav %}
{% endmacro %}

So I think all told that should be no more than three calls to craft.navigation...: once for the main menu, once for the mobile donation button, and once for the footer sitemap.

engram-design commented 2 years ago

So the only thing here would be the node.children.all() which are their own queries, which is likely where it's coming from. Would you say you have maybe ~15 subnodes? Using the {% nav %} tag helps a lot with this sort of thing, but isn't always feasible to use.

But regardless, I'll see what I can do about caching this check. It's not a massive performance hit with that call (and not an issue for Craft 4).

proimage commented 2 years ago

One thing I just noticed is that this is happening on my local dev environment, but not on production. So possibly a non-issue I guess?

Edit: There's 19 subnodes... EDIT 2: actually, I counted in the backend and there's 22. 🤪

engram-design commented 2 years ago

Hmmm, possibly a non-issue for sure. And it'd be the count of any nodes with subnodes, not just the number of subnodes.

But anyway, I might close this one, unless it's really bugging you!