launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01485
[Merge] lp:~wallyworld/launchpad/improve-menu-rendering into lp:launchpad/devel
Ian Booth has proposed merging lp:~wallyworld/launchpad/improve-menu-rendering into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
#659171 Menu performance fix - only render required links
https://bugs.launchpad.net/bugs/659171
The Problem
----------------
When rendering pages/views, there are performance issues in the way menus are handled. Three relevant examples:
1. Many templates define an instance of a menu at the start of the markup to avoid the cost of duplicate instances, but we know that the menu will be reinstantiated in a portlet. Creating reusable markup often means death by menu queries
2. canonical.launchpad.webapp.MenuBase class eagerly iterates over all menu links during setup (and hence executes all required underlying database queries to construct the link) even if the links are not actually rendered.
3. There's a _has_factet() call which is called many times during a page render. Each one of these calls iterlinks() on the menu and the only thing it does then is to look at the link names. These are known without having to go through and unnecessarily create all the menu links each time, over and over again.
Implementation
--------------------
This branch modifies menu infrastructure so that only the menu links are only instantiated as required by the page template. This solves the major performance issue and means the impact of creating the same menu instances more than once are eliminated for all practical purposes.
The fix includes changes to:
lp/app/browser/tales.py
Add new MenuLinksDict class to hold relevant menu details and create ILink instances as required
canonical/launchpad/webapp/menu.py
Modify menu processing to avoid eagerly instantiating all links when menu instance is constructed
Refactor _has_facet() to not create menu links everytime
Tests
------
bin/test -vvt menus
Regression testing of PersonBranchListing pages
There was an issue with a doc test for broken links.
A zope source file from the page template egg - zope.pagetemplate.engine.py - contains this code in class ZopeTraverser:
if getattr(object, '__class__', None) == dict:
This check now fails since the menu object is now MenuLinksDict (which subclasses dict). If the above check had been written using isinstance() instead, it wouldn't have been an issue. So what happens now is that a LocationError instead of a KeyError is raised when a bad menu link is rendered. The doc test was modified to handle this. I'm not aware of any production coding issues which will be impacted by the different exception type.
Lint
----
bin/lint.sh
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/launchpad/doc/menus.txt
lib/canonical/launchpad/webapp/menu.py
lib/lp/app/browser/tales.py
./lib/canonical/launchpad/doc/menus.txt
1: narrative uses a moin header.
6: narrative uses a moin header.
55: narrative uses a moin header.
102: source exceeds 78 characters.
164: narrative uses a moin header.
251: narrative uses a moin header.
331: source exceeds 78 characters.
361: narrative uses a moin header.
420: narrative uses a moin header.
491: narrative uses a moin header.
557: narrative uses a moin header.
565: source exceeds 78 characters.
677: narrative uses a moin header.
870: narrative uses a moin header.
889: narrative uses a moin header.
901: narrative uses a moin header.
Process finished with exit code 0
--
https://code.launchpad.net/~wallyworld/launchpad/improve-menu-rendering/+merge/38222
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/improve-menu-rendering into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/doc/menus.txt'
--- lib/canonical/launchpad/doc/menus.txt 2009-10-21 17:41:20 +0000
+++ lib/canonical/launchpad/doc/menus.txt 2010-10-12 14:01:11 +0000
@@ -810,12 +810,15 @@
>>> print link.url
http://launchpad.dev/sesamestreet/number73/+first
-But if a non-existing entry is requested, a KeyError is raised:
+But if a non-existing entry is requested, a LocationError is raised. This used
+to raise a KeyError. But the menu links are now no longer stored as a dict and
+there's zope code which is hard coded to check specifically for a dict in
+order to use a short circuit.
>>> test_tales('view/menu:foo/broken', view=view)
Traceback (most recent call last):
...
- KeyError: 'broken'
+ LocationError: ..., 'broken')
We also report when the selected facet does not exist with a
LocationError exception:
=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/webapp/menu.py 2010-10-12 14:01:11 +0000
@@ -100,7 +100,7 @@
# among the traversed_names. We need to get it from a private attribute.
view = request._last_obj_traversed
# Note: The last traversed object may be a view's instance method.
- bare = removeSecurityProxy(view)
+ bare = removeSecurityProxy(view)
if zope_isinstance(view, types.MethodType):
return bare.im_self
return bare
@@ -235,7 +235,7 @@
_initialized = False
_forbiddenlinknames = set(
['user', 'initialize', 'links', 'enable_only', 'isBetaUser',
- 'iterlinks', 'extra_attributes'])
+ 'iterlinks', 'initLink', 'updateLink', 'extra_attributes'])
def __init__(self, context):
# The attribute self.context is defined in IMenuBase.
@@ -246,6 +246,12 @@
"""Override this in subclasses to do initialization."""
pass
+ def _check_links(self):
+ assert self.links is not None, (
+ 'Subclasses of %s must provide self.links' % self._baseclassname)
+ assert isinstance(self.links, (tuple, list)), (
+ "self.links must be a tuple or list.")
+
def _buildLink(self, name):
method = getattr(self, name, None)
# Since Zope traversals hides the root cause of an AttributeError,
@@ -281,15 +287,12 @@
except KeyError:
raise AssertionError('unknown site', site)
- def iterlinks(self, request_url=None):
- """See IMenu."""
- if not self._initialized:
- self.initialize()
- self._initialized = True
- assert self.links is not None, (
- 'Subclasses of %s must provide self.links' % self._baseclassname)
- assert isinstance(self.links, (tuple, list)), (
- "self.links must be a tuple or list.")
+ def _init_link_data(self):
+ if self._initialized:
+ return
+ self._initialized = True
+ self.initialize()
+ self._check_links()
links_set = set(self.links)
assert not links_set.intersection(self._forbiddenlinknames), (
"The following names may not be links: %s" %
@@ -303,14 +306,14 @@
else:
context = self.context
- contexturlobj = URI(canonical_url(context))
+ self._contexturlobj = URI(canonical_url(context))
if self.enable_only is ALL_LINKS:
- enable_only_set = links_set
+ self._enable_only_set = links_set
else:
- enable_only_set = set(self.enable_only)
+ self._enable_only_set = set(self.enable_only)
- unknown_links = enable_only_set - links_set
+ unknown_links = self._enable_only_set - links_set
if len(unknown_links) > 0:
# There are links named in enable_only that do not exist in
# self.links.
@@ -318,36 +321,53 @@
"Links in 'enable_only' not found in 'links': %s" %
', '.join(sorted(unknown_links)))
- for idx, linkname in enumerate(self.links):
- link = self._get_link(linkname)
- link.name = linkname
-
- # Set the .enabled attribute of the link to False if it is not
- # in enable_only.
- if linkname not in enable_only_set:
- link.enabled = False
-
- # Set the .url attribute of the link, using the menu's context.
- if link.site is None:
- rootsite = contexturlobj.resolve('/')
+ def initLink(self, linkname, request_url=None):
+ self._init_link_data()
+ link = self._get_link(linkname)
+ link.name = linkname
+
+ # Set the .enabled attribute of the link to False if it is not
+ # in enable_only.
+ if linkname not in self._enable_only_set:
+ link.enabled = False
+
+ # Set the .url attribute of the link, using the menu's context.
+ if link.site is None:
+ rootsite = self._contexturlobj.resolve('/')
+ else:
+ rootsite = self._rootUrlForSite(link.site)
+ # Is the target a full URI already?
+ try:
+ link.url = URI(link.target)
+ except InvalidURIError:
+ if link.target.startswith('/'):
+ link.url = rootsite.resolve(link.target)
else:
- rootsite = self._rootUrlForSite(link.site)
- # Is the target a full URI already?
- try:
- link.url = URI(link.target)
- except InvalidURIError:
- if link.target.startswith('/'):
- link.url = rootsite.resolve(link.target)
- else:
- link.url = rootsite.resolve(contexturlobj.path).append(
- link.target)
-
- # Make the link unlinked if it is a link to the current page.
- if request_url is not None:
- if request_url.ensureSlash() == link.url.ensureSlash():
- link.linked = False
-
- link.sort_key = idx
+ link.url = rootsite.resolve(self._contexturlobj.path).append(
+ link.target)
+
+ # Make the link unlinked if it is a link to the current page.
+ if request_url is not None:
+ if request_url.ensureSlash() == link.url.ensureSlash():
+ link.linked = False
+
+ idx = self.links.index(linkname)
+ link.sort_key = idx
+ return link
+
+ def updateLink(self, link, request_url, **kwargs):
+ """Called each time a link is rendered.
+
+ Override to update the link state as required for the given request.
+ """
+ pass
+
+ def iterlinks(self, request_url=None, **kwargs):
+ """See IMenu."""
+ self._check_links()
+ for linkname in self.links:
+ link = self.initLink(linkname, request_url)
+ self.updateLink(link, request_url, **kwargs)
yield link
@@ -369,16 +389,18 @@
return IFacetLink(
self._filterLink(name, MenuBase._get_link(self, name)))
- def iterlinks(self, request_url=None, selectedfacetname=None):
- """See IFacetMenu."""
+ def initLink(self, linkname, request_url=None):
+ link = super(FacetMenu, self).initLink(linkname, request_url)
+ link.url = link.url.ensureNoSlash()
+ return link
+
+ def updateLink(self, link, request_url=None, selectedfacetname=None):
+ super(FacetMenu, self).updateLink(link, request_url)
if selectedfacetname is None:
selectedfacetname = self.defaultlink
- for link in super(FacetMenu, self).iterlinks(request_url=request_url):
- if (selectedfacetname is not None and
- selectedfacetname == link.name):
- link.selected = True
- link.url = link.url.ensureNoSlash()
- yield link
+ if (selectedfacetname is not None and
+ selectedfacetname == link.name):
+ link.selected = True
class ApplicationMenu(MenuBase):
@@ -407,6 +429,20 @@
title = None
disabled = False
+ def initLink(self, linkname, request_url):
+ link = super(NavigationMenu, self).initLink(linkname, request_url)
+ link.url = link.url.ensureNoSlash()
+ return link
+
+ def updateLink(self, link, request_url=None, view=None):
+ super(NavigationMenu, self).updateLink(link, request_url)
+ # The link should be unlinked if it is the current URL, or if
+ # the menu for the current view is the link's menu.
+ if view is None:
+ view = get_current_view(self.request)
+ link.linked = not (self._is_current_url(request_url, link.url)
+ or self._is_menulink_for_view(link, view))
+
def iterlinks(self, request_url=None):
"""See `INavigationMenu`.
@@ -419,12 +455,8 @@
if request_url is None:
request_url = URI(request.getURL())
- for link in super(NavigationMenu, self).iterlinks():
- # The link should be unlinked if it is the current URL, or if
- # the menu for the current view is the link's menu.
- link.url = link.url.ensureNoSlash()
- link.linked = not (self._is_current_url(request_url, link.url)
- or self._is_menulink_for_view(link, view))
+ for link in super(NavigationMenu, self).iterlinks(
+ request_url=request_url, view=view):
yield link
def _is_current_url(self, request_url, link_url):
@@ -487,6 +519,7 @@
called.
"""
permission = self.permission
+
def enable_if_allowed(self):
link = func(self)
if not check_permission(permission, self.context):
@@ -504,7 +537,6 @@
# This entire block should be extracted into its own module, along
# with the structured() class.
-
def escape(message):
"""Performs translation and sanitizes any HTML present in the message.
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py 2010-10-03 15:30:06 +0000
+++ lib/lp/app/browser/tales.py 2010-10-12 14:01:11 +0000
@@ -68,16 +68,74 @@
IDistributionSourcePackage,
)
from lp.registry.interfaces.person import IPerson
-from lp.registry.interfaces.product import (
- IProduct,
- LicenseStatus,
- )
+from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
SEPARATOR = ' : '
+class MenuLinksDict(dict):
+ """A dict class to construct menu links when asked for and not before.
+
+ We store all the information we need to construct the requested links,
+ including the menu object and request url.
+ """
+
+ def __init__(self, menu, request_url, request):
+ self._request_url = request_url
+ self._menu = menu
+ self._all_link_names = []
+ self._extra_link_names = []
+ dict.__init__(self)
+
+ # The object has the facet, but does not have a menu, this
+ # is probably the overview menu with is the default facet.
+ if menu is None or getattr(menu, 'disabled', False):
+ return
+ menu.request = request
+
+ # We get all the possible link names for the menu.
+ # The link names are the defined menu links plus any extras.
+ self._all_link_names = list(menu.links)
+ extras = menu.extra_attributes
+ if extras is not None:
+ self._extra_link_names = list(extras)
+ self._all_link_names.extend(extras)
+
+ def __getitem__(self, link_name):
+ if not link_name in self._all_link_names:
+ raise KeyError(link_name)
+
+ if link_name in self.keys():
+ link = dict.__getitem__(self, link_name)
+ else:
+ if link_name in self._extra_link_names:
+ link = getattr(self._menu, link_name, None)
+ else:
+ link = self._menu.initLink(link_name, self._request_url)
+
+ if not link_name in self._extra_link_names:
+ self._menu.updateLink(link, self._request_url)
+
+ self[link_name] = link
+ return link
+
+ def __delitem__(self, key):
+ self._all_link_names.remove(key)
+ dict.__delitem__(self, key)
+
+ def items(self):
+ return zip(self._all_link_names, self.values())
+
+ def values(self):
+ return [self[key] for key in self._all_link_names]
+
+ def iterkeys(self):
+ return iter(self._all_link_names)
+ __iter__ = iterkeys
+
+
class MenuAPI:
"""Namespace to give access to the facet menus.
@@ -131,34 +189,20 @@
menu = queryAdapter(self._context, IApplicationMenu, facet)
if menu is None:
menu = queryAdapter(self._context, INavigationMenu, facet)
- if menu is not None:
- links_map = self._getMenuLinksAndAttributes(menu)
- else:
- # The object has the facet, but does not have a menu, this
- # is probably the overview menu with is the default facet.
- links_map = {}
- object.__setattr__(self, facet, links_map)
- return links_map
+ links = self._getMenuLinksAndAttributes(menu)
+ object.__setattr__(self, facet, links)
+ return links
def _getMenuLinksAndAttributes(self, menu):
"""Return a dict of the links and attributes of the menu."""
- menu.request = self._request
- request_url = self._request_url()
- result = dict(
- (link.name, link)
- for link in menu.iterlinks(request_url=request_url))
- extras = menu.extra_attributes
- if extras is not None:
- for attr in extras:
- result[attr] = getattr(menu, attr, None)
- return result
+ return MenuLinksDict(menu, self._request_url(), self._request)
def _has_facet(self, facet):
"""Does the object have the named facet?"""
- for facet_entry in self.facet():
- if facet == facet_entry.name:
- return True
- return False
+ menu = self._facet_menu()
+ if menu is None:
+ return False
+ return facet in menu.links
def _request_url(self):
request = self._request
@@ -176,6 +220,16 @@
return request_urlobj
def facet(self):
+ """Return the IFacetMenu links related to the context."""
+ menu = self._facet_menu()
+ if menu is None:
+ return []
+ menu.request = self._request
+ return list(menu.iterlinks(
+ request_url=self._request_url(),
+ selectedfacetname=self._selectedfacetname))
+
+ def _facet_menu(self):
"""Return the IFacetMenu related to the context."""
try:
try:
@@ -188,12 +242,7 @@
except NoCanonicalUrl:
menu = None
- if menu is None:
- return []
- menu.request = self._request
- return list(menu.iterlinks(
- request_url=self._request_url(),
- selectedfacetname=self._selectedfacetname))
+ return menu
def selectedfacetname(self):
if self._selectedfacetname is None:
@@ -204,10 +253,7 @@
@property
def context(self):
menu = IContextMenu(self._context, None)
- if menu is None:
- return {}
- else:
- return self._getMenuLinksAndAttributes(menu)
+ return self._getMenuLinksAndAttributes(menu)
@property
def navigation(self):
@@ -229,10 +275,7 @@
context, INavigationMenu, name=selectedfacetname)
except NoCanonicalUrl:
menu = None
- if menu is None or menu.disabled:
- return {}
- else:
- return self._getMenuLinksAndAttributes(menu)
+ return self._getMenuLinksAndAttributes(menu)
except AttributeError, e:
# If this method gets an AttributeError, we rethrow it as a
# AssertionError. Otherwise, zope will hide the root cause
Follow ups
