Query count reduction #47

Closed
wants to merge 13 commits into
from

Projects

None yet

2 participants

@spookylukey
Contributor

These commits reduce the query count for the example project from 30 to 23 (for the home page, logged out).

One of the queries removed is a query in show_menu that loaded almost the entire Page table:

SELECT "fiber_page"."id", "fiber_page"."created", "fiber_page"."updated", "fiber_page"."parent_id", "fiber_page"."title", "fiber_page"."url", "fiber_page"."redirect_page_id", "fiber_page"."mark_current_regexes", "fiber_page"."template_name", "fiber_page"."show_in_menu", "fiber_page"."is_public", "fiber_page"."protected", "fiber_page"."metadata", "fiber_page"."lft", "fiber_page"."rght", "fiber_page"."tree_id", "fiber_page"."level" FROM "fiber_page" WHERE ("fiber_page"."is_public" = True AND "fiber_page"."show_in_menu" = True ) ORDER BY "fiber_page"."tree_id" ASC, "fiber_page"."lft" ASC

(Note the lack of any LIMIT here).

I'm also going to have a go at show_menu (see issue #12), but that will take more work.

spookylukey added some commits Jan 16, 2012
@spookylukey spookylukey Reduced the number of queries needed by show_page_content 720ce6b
@spookylukey spookylukey Removed some unnecessary queries due to PageManager methods.
PageManager.public_for_user:

First, doing 'obj in qs' is *extremely* inefficient if, as in this case in
the context processor, you are discarding the results of qs.

Second, an additional query here was not helpful, since the objects had
already been fully fetched from the database.  An object-level method was
therefore preferable, and since PageManager.public_for_user() was not used
elsewhere, this was removed and replaced with Page.is_public_for_user()

PageManager.shown_in_menu_for_user:

Similar to above, in the context of show_menu, using this method
just does additional unnecessary queries, when all the needed
information has already been fetched.

In addition, shown_in_menu_for_user() had a bug - for staff users it showed
all Pages in the menu, even if they had 'show_in_menu = False', which surely
was not intended.

Since shown_in_menu_for_user was not used elsewhere, it was removed, and
also PageManager since its only two methods have been removed.
302fa5c
@spookylukey spookylukey Refactored menu tests in preparation for expanding them. 285b0dc
@spookylukey spookylukey Added more data for menu tests, and fixed tests accordingly 1258449
@spookylukey spookylukey Added tests for show_menu with "all_descendants" 8031cab
@spookylukey spookylukey Added tests for minimum and maximum level for show_menu 6bbbb13
@spookylukey spookylukey Added test for rendering menu when on current page is a different root 0a708ea
@spookylukey spookylukey Rewrote show_menu template tag to be much more efficient in use of qu…
…eries.
f3481c0
@spookylukey spookylukey Optimization of looking up Page by URL, reducing it to 2 or 3 queries…
… maximum.
2519472
@spookylukey spookylukey Optimized Page.get_ancestors for the case where we have already retri…
…eved the chain of ancestors

This has big effects on the number of queries produced due to the page_info context_processor,
especially if the items in 'current_pages' have 'get_absolute_url' called (e.g. bread crumb).
3e3e74e
@spookylukey
Contributor

I did a bunch more, including fixes for issue #12 and tests to ensure I didn't introduce bugs.

To get an idea of the improvements, for the django-fiber-example project, here are some samples of changes in query count:

Page: /

from 30 to 18 (logged out)
from 103 to 43 (logged in)

Page: /products/product-b/downloads/

from 63 to 20 (logged out)
from 150 to 47 (logged in)

I've also been careful to reduce the size of the queries (in terms of the amount of data returned), not just the number. Some of the queries have obviously got more complex due to this, especially in show_menu.

@spookylukey
Contributor

Further reductions: another 3 or 4 queries per request eliminated.

spookylukey added some commits Jan 17, 2012
@spookylukey spookylukey Fixed bug where an empty list stored in ContentItem.used_on_pages_dat…
…a was being ignored.

This resulted in lots of UPDATE queries for unused ContentItems being
generated every time a page was viewed when logged in.
1b920bb
@spookylukey spookylukey Tightened tests for show_menu to check query count in all cases d66dd9c
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Avoided creating two RequestContexts per request, which was causing d…
…uplicated queries

- Fixes sub issue of Issue #47: Query count reduction
4a0667a
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Reduced the number of queries needed by show_page_content
- Fixes sub issue of Issue #47: Query count reduction
749ad50
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter + michael Removed some unnecessary queries due to PageManager methods.
- Fixes sub issue of Issue #47: Query count reduction
- Bugfix shown_in_menu_for_user() for staff users
385f052
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Refactored menu tests in preparation for expanding them.
- Fixes sub issue of Issue #47: Query count reduction
f9ac7b3
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Added more data for menu tests, and fixed tests accordingly
- Fixes sub issue of Issue #47: Query count reduction
78b1324
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Added tests for show_menu with "all_descendants"
- Fixes sub issue of Issue #47: Query count reduction
0094480
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Added tests for minimum and maximum level for show_menu
- Fixes sub issue of Issue #47: Query count reduction
ddca33d
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Added test for rendering menu when on current page is a different root
- Fixes sub issue of Issue #47: Query count reduction
07c8c9c
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Rewrote show_menu template tag to be much more efficient in use of qu…
…eries.

- Fixes sub issue of Issue #47: Query count reduction
- Fixes sub issue of Issue #12: Too many DB queries
d8f3b34
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Optimization of looking up Page by URL, reducing it to 2 or 3 queries…
… maximum.

- Fixes sub issue of Issue #47: Query count reduction
4036b6e
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Optimized Page.get_ancestors for the case where we have already retri…
…eved the chain of ancestors.

- Fixes sub issue of Issue #47: Query count reduction
42e2931
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Fixed bug where an empty list stored in ContentItem.used_on_pages_dat…
…a was being ignored.

- Fixes sub issue of Issue #47: Query count reduction
994c87e
@mvdwaeter mvdwaeter added a commit that referenced this pull request Jan 19, 2012
@mvdwaeter mvdwaeter Tightened tests for show_menu to check query count in all cases
- Fixes sub issue of Issue #47: Query count reduction
ca8a43d
@dbunskoek
Member

We merged your pull request, and it shows a nice average improvement in page load time of approximately 30% (tested on some of our live sites / apps).
The newest version of Django Fiber (0.9.6.2) contains these improvements, as well as the removal of the PageFallbackMiddleware, in favor of the catch-all url.
Thanks for providing this pull request, and for the constructive criticism in your blog post :)

@dbunskoek dbunskoek closed this Jan 19, 2012
@spookylukey
Contributor

My pleasure - thanks for the nice CMS, BTW, it's just right for some projects I've created and have in mind, and the code was pretty easy to work on :-)

@smyrman smyrman referenced this pull request Jan 29, 2012
Closed

i18n #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment