search

jazzband / wagtailmenus

An app to help you manage and render menus in your Wagtail projects more effectively
MIT License
398 stars 138 forks source link

Disable context processor for wagtail admin pages #385

Closed cazgp closed 3 years ago

cazgp commented 3 years ago

On the wagtail admin page with just one page, there were 103 database requests made due to the context processors. Disabling this, there are now only 25.

ababic commented 3 years ago

@cazgp I have a feeling you may have misdiagnosed the problem here. This context processor is 'lazy' - it doesn't make any database queries unless wagtailmenus_vals is evaluated, which should only happen in wagtailmenus' menu_tags in a production site - which are highly unlikely to be used in the wagtail admin area. Even if they were, there's no part of the context processor that would generate 75+ database queries.

cazgp commented 3 years ago

@ababic I've definitely not misdiagnosed that there is a problem, as enacting this code change fixed the issues I was seeing (otherwise I wouldn't have put up the PR).

I've re-run the tests and the highest number of queries difference I got was 73, so apologies for the bad maths... but that doesn't negate that there is an issue.

Try running with django-debug-toolbar and see for yourself.

Here are the results of my tests (which took just a few minutes to run).

Root Admin page

From the root /admin/ page before applying the fix, after refreshing multiple times to ensure the queries were cached:

2020-12-28-171642_217x64_scrot

From the root /admin/ page after applying the fix, after refreshing multiple times to ensure caching:

2020-12-28-171754_220x65_scrot

The fix prevented 32 queries from being run.

Viewing a page

Go to the sidebar, click "Pages" and go to the homepage.

Before (after caching):

2020-12-28-172317_220x68_scrot

After (after caching):

2020-12-28-172359_220x66_scrot

The fix prevented 61 queries from being run.

Add page

Click "Add a child page".

Before:

2020-12-28-172638_221x67_scrot

After:

2020-12-28-172719_219x65_scrot

The fix prevents 73 queries from being run.

Without wagtailmenus installed

To ensure it's nothing that I'm doing wrong, I commented out wagtailmenus and clicked "Add child page" and got the following:

2020-12-28-172958_220x66_scrot

TL;DR

There's definitely something up with wagtailmenus making too many database queries.

If you suspect this isn't the right place to fix the problem, then by all means make a suggestion as to where, but please don't ignore that there is a problem.

ababic commented 3 years ago

Apologies @cazp. It was impulsive of me to close this without investigating further. I will give things a look, but I've found debug toolbar to be a bit of a double-edged sword... It can give you useful data, but it can also muddy the waters slightly, with some panels forcing evaluation of objects that wouldn't normally be evaluated - triggering additional queries as a result. Either way, it can only be a good thing to understand why you're seeing what you're seeing, and see what can be done about it.

ababic commented 3 years ago

Hi @cazgp. Could you tell me whether you still see these extra queries being made when only the SQL panel is enabled? It should just be a case of updating the DEBUG_TOOLBAR_PANELS setting as follows:

DEBUG_TOOLBAR_PANELS = [
    "debug_toolbar.panels.sql.SQLPanel",
]
cazgp commented 3 years ago

Hi @ababic thanks for investigating! When only the SQL panel is enabled, no the problem doesn't exist -- it's 72 queries uncached and 66 cached.

Which panel do you think is causing it? It seems like it's still problematic though because whatever is triggering that context function is still making too many queries...

ababic commented 3 years ago

@cazp My guess would be debug_toolbar.panels.profiling.ProfilingPanel - Although even with that enabled locally, I'm not seeing any rise in the number of queries within the admin area (Django 3.0.11, Wagtail 2.10.2, Wagtailmenus 3.0.2, Django Debug Toolbar 3.2)

cazgp commented 3 years ago

It's not the Profiling Panel, but "debug_toolbar.panels.templates.TemplatesPanel" (readded them one at a time and refreshed)

ababic commented 3 years ago

Okay, I have a hunch about what is happening. I'm guessing the WAGTAILMENUS_GUESS_TREE_POSITION_FROM_PATH setting is the default value (True)? If so, the Templates panel is causing the (usually lazy) wagtailmenus_vals value to be evaluated multiple times - once for every HTML template that is loaded during the rendering process. And each time that happens, queries are being made to attempt to identify the current/closest page from the request URL.

cazgp commented 3 years ago

Given that I've never seen that value before, it is definitely the default! So that hunch seems reasonable to me.

If that is the case, does disabling the context processor the way I have make sense, or do you think there's a better fix which can be made?

ababic commented 3 years ago

@cazgp It would seem that whether or not you are looking at a wagtail admin URL is really irrelevant here (it's just where the issue is more apparent, due to the number of templates being used), so I think the best thing to do is close this PR and go about your day. You can easily disable that panel locally if it's doing more harm than good - or perhaps adding to the conversation on the following issue would help prompt a solution: https://github.com/jazzband/django-debug-toolbar/issues/1136