perfs: optimize extend_with_parent_cells (#21052)

combo/data/models.py

@@ -835,22 +835,29 @@ class ParentContentCell(CellBase):
     class Meta:
         verbose_name = _('Same as parent')
 
-    def get_cells(self):
-        if self.page.parent:
-            parent_page = self.page.parent
-        elif self.page.slug != 'index':
-            try:
-                parent_page = Page.objects.get(slug='index', parent=None)
-            except Page.DoesNotExist:
-                return []
-        else:
+    def get_parents_cells(self, hierarchy):
+        try:
+            page_ids = [Page.objects.get(slug='index', parent=None).id]
+        except Page.DoesNotExist:
+            page_ids = []
+        page_ids.extend([x.id for x in hierarchy])
+        if not page_ids:
             return []
-        cells = CellBase.get_cells(placeholder=self.placeholder, page=parent_page)
-        for i, cell in enumerate(cells):
-            if not isinstance(cell, ParentContentCell):
-                continue
-            cells[i:i+1] = cell.get_cells()
-            break
+        cells_by_page = {}
+        for page_id in page_ids:
+            cells_by_page[page_id] = []
+        for cell in list(CellBase.get_cells(placeholder=self.placeholder, page__in=page_ids)):
+            cells_by_page[cell.page_id].append(cell)
+
+        cells = cells_by_page[page_ids[-1]]
+        for page_id in reversed(page_ids[:-1]):
+            for i, cell in enumerate(cells):
+                if isinstance(cell, ParentContentCell):
+                    cells[i:i+1] = cells_by_page[page_id]
+                    break
+            else:
+                # no more ParentContentCell, stop folloing the parent page chain
+                break
         return cells
 
     def render(self, context):

combo/public/views.py

@@ -125,12 +125,12 @@ def render_cell(request, cell):
     return HttpResponse(template.render(context, request), content_type='text/html')
 
 
-def extend_with_parent_cells(cells):
+def extend_with_parent_cells(cells, hierarchy):
     for cell in cells[:]:
         if not isinstance(cell, ParentContentCell):
             continue
         idx = cells.index(cell)
-        cells[idx:idx+1] = cell.get_cells()
+        cells[idx:idx+1] = cell.get_parents_cells(hierarchy=hierarchy[:-1])
 
 
 def should_check_badges():
@@ -224,7 +224,7 @@ def skeleton(request):
 
     pages = selected_page.get_parents_and_self()
     combo_template = settings.COMBO_PUBLIC_TEMPLATES[selected_page.template_name]
-    extend_with_parent_cells(cells)
+    extend_with_parent_cells(cells, hierarchy=pages)
 
     ctx = {
         'page': selected_page,
@@ -357,7 +357,7 @@ def publish_page(request, page, status=200, template_name=None):
         return HttpResponseRedirect(page.get_redirect_url())
 
     cells = CellBase.get_cells(page_id=page.id)
-    extend_with_parent_cells(cells)
+    extend_with_parent_cells(cells, hierarchy=pages)
     cells = [x for x in cells if x.is_visible(user=request.user)]
 
     ctx = {

tests/test_public.py

@@ -10,7 +10,9 @@ from urlparse import urlparse
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from django.db import connection
 from django.test import override_settings
+from django.test.utils import CaptureQueriesContext
 
 from combo.wsgi import application
 from combo.data.models import (Page, CellBase, TextCell, ParentContentCell,
@@ -67,15 +69,47 @@ def test_page_footer_acquisition(app):
     cell = TextCell(page=page, placeholder='footer', text='BARFOO', order=0)
     cell.save()
     resp = app.get('/', status=200)
-    assert 'BARFOO' in resp.body
+    assert resp.body.count('BARFOO') == 1
 
     page = Page(title='Second', slug='second', template_name='standard')
     page.save()
     ParentContentCell(page=page, placeholder='footer', order=0).save()
+    TextCell(page=page, placeholder='footer', text='BAR2FOO', order=1).save()
     resp = app.get('/second', status=301)
     assert urlparse(resp.location).path == '/second/'
-    resp = app.get('/second/', status=200)
-    assert 'BARFOO' in resp.body
+    with CaptureQueriesContext(connection) as ctx:
+        resp = app.get('/second/', status=200)
+        assert resp.body.count('BARFOO') == 1
+        assert resp.body.count('BAR2FOO') == 1
+        queries_count_second = len(ctx.captured_queries)
+
+    # deeper hierarchy
+    page3 = Page(title='Third', slug='third', template_name='standard', parent=page)
+    page3.save()
+    ParentContentCell(page=page3, placeholder='footer', order=0).save()
+
+    page4 = Page(title='Fourth', slug='fourth', template_name='standard', parent=page3)
+    page4.save()
+    ParentContentCell(page=page4, placeholder='footer', order=0).save()
+
+    page5 = Page(title='Fifth', slug='fifth', template_name='standard', parent=page4)
+    page5.save()
+    ParentContentCell(page=page5, placeholder='footer', order=0).save()
+
+    # check growth in SQL queries is limited to an additional page lookup
+    with CaptureQueriesContext(connection) as ctx:
+        resp = app.get('/second/third/', status=200)
+        assert resp.body.count('BARFOO') == 1
+        assert resp.body.count('BAR2FOO') == 1
+        queries_count_third = len(ctx.captured_queries)
+        assert queries_count_third == queries_count_second + 1
+
+    with CaptureQueriesContext(connection) as ctx:
+        resp = app.get('/second/third/fourth/', status=200)
+        assert resp.body.count('BARFOO') == 1
+        assert resp.body.count('BAR2FOO') == 1
+        queries_count_fourth = len(ctx.captured_queries)
+        assert queries_count_fourth == queries_count_second + 2
 
 def test_page_redirect(app):
     Page.objects.all().delete()