Launchpad itself

Merge lp:~salgado/launchpad/cached-menus into lp:launchpad

  1. cached-menus
  2. Merge into devel
Proposed by Guilherme Salgado
Status: Merged
Merged at revision: not available
Proposed branch: lp:~salgado/launchpad/cached-menus
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~salgado/launchpad/cached-menus
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Canonical Launchpad Engineering Pending
Review via email: mp+9599@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Guilherme Salgado (salgado) wrote :

= Summary =

Some of our pages do stuff like context/menu:bugs/foo multiple times, and
every time they do that, our code will generate the whole list of Links that
compose that menu. Some of these Links may hit the DB (for various reasons),
thus making these pages more expensive and slower than they need to be, so
this branch will cache these Links in the request to avoid the overhead when
they're re-generated in the same request.

== Proposed fix ==

The security checks are already cached in the request, so that's not what
makes it expensive to re-generate the list of Links for a given facet.

What seems to cause that are the links that do expensive calculations to
figure out whether or not they should be enabled. These are always triggered
by the <ApplicationMenu>.linkname() method that returns the Link object, so we
could easily change MenuBase._get_link() to cache the result of the method
call in the request and use it when it is available.

Most of the steps done in MenuBase.iterlinks() can also be avoided if we
refactor that method, moving the bulk of it to _get_link(), but that won't
really buy us much, since as said above, the expensive operations are the ones
that hit the DB, triggered by the AppMenu.linkname() calls.

== Implementation details ==

There's an XXX about the use of a regular dict instead of weak refs in
the cache here. We need the former here, but I want to double check it's
ok to use them.

== Tests ==

./bin/test -vvt test_menu

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/webapp/tests/test_menu.py
  lib/canonical/launchpad/webapp/menu.py

== Pylint notices ==

lib/canonical/launchpad/webapp/menu.py
    42: [F0401] Unable to import 'lazr.uri' (No module named uri)

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks for doing this.

As we discussed in our call, we will not use a weakref dict. A dict will not be garbage collected as quickly.

review: Approve (code)
Revision history for this message
Guilherme Salgado (salgado) wrote :
Download full text (6.4 KiB)

The key used in the original implementation is not unique, as it doesn't include the context. Also, the key uses a class attribute (_baseclassname) which is not necessarily defined in subclasses, so should not be used there.

The diff below addresses these two issues and also fixes one test that was using a FakeRequest which didn't provide the 'annotations' attribute, so the new code was crashing when attempting to cache stuff.

=== modified file 'lib/canonical/launchpad/doc/menus.txt'
--- lib/canonical/launchpad/doc/menus.txt 2009-06-11 16:02:25 +0000
+++ lib/canonical/launchpad/doc/menus.txt 2009-08-05 15:39:02 +0000
@@ -339,7 +339,6 @@
     # We need to use a real launchpad test request so the view adapter
     # lookups will work. That request also needs to implement
     # IParticipation so that the login machinery will work.
-
     >>> from canonical.launchpad.webapp.servers import LaunchpadTestRequest
     >>> from zope.security.interfaces import IParticipation
     >>> class InteractiveTestRequest(LaunchpadTestRequest):
@@ -354,7 +353,6 @@
     <a class="menu-link-None sprite icon" title="summary">text--&gt;</a>

     # Clean up our special login.
-
     >>> login(ANONYMOUS)

@@ -436,6 +434,7 @@
     ... self.query = query
     ... self.url1 = url1 # returned from getURL(1)
     ... self.method = 'GET'
+ ... self.annotations = {}
     ...
     ... def getURL(self, level=0):
     ... assert 0 <= level <=1, 'level must be 0 or 1'
@@ -470,9 +469,9 @@
 The following request is for a url that is different from all of the
 facet menu links for 'sesamestreet'. So, all links will be linked.

- >>> request = URI('http://launchpad.dev/sesamestreet/+xxx')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+xxx')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar True
@@ -480,19 +479,19 @@
 This next request is for +foo, but with a query string. So, both links
 will still be linked.

- >>> request = FakeRequest(
- ... 'http://launchpad.dev/sesamestreet/+foo', query="123")
+ >>> request_uri = URI(
+ ... 'http://launchpad.dev/sesamestreet/+foo?a=123')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks():
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar True

 Next, we'll use the link +bar. The +bar link will not be linked.

- >>> request = URI('http://launchpad.dev/sesamestreet/+bar')
+ >>> request_uri = URI('http://launchpad.dev/sesamestreet/+bar')
     >>> facets = Facets(street)
- >>> for link in facets.iterlinks(request):
+ >>> for link in facets.iterlinks(request_uri):
     ... print link.url, link.linked
     http://launchpad.dev/sesamestreet/+foo True
     http://launchpad.dev/sesamestreet/+bar False
@@ -500,9 +499,9 @@
 Now, we use +bar with a trailing slash. The +...

Read more...

Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Salgado.

This was an interesting problem.

As we agreed on IRC, the facet menu test can be removed because lazr/doc/menus.txt also tests the feature, and it clearly tests it the right way. I supplied a patch that moves the absolute URL test changes into lazr/doc/menus.txt. The launchpad/doc/menus.txt is and old and bad test that we are trying to remove.

Your implementation and real test is fine.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/webapp/menu.py'
--- lib/canonical/launchpad/webapp/menu.py 2009-07-17 00:26:05 +0000
+++ lib/canonical/launchpad/webapp/menu.py 2009-08-03 20:38:34 +0000
@@ -197,6 +197,8 @@
197# Marker object that means 'all links are to be enabled'.197# Marker object that means 'all links are to be enabled'.
198ALL_LINKS = object()198ALL_LINKS = object()
199199
200MENU_ANNOTATION_KEY = 'canonical.launchpad.webapp.menu.links'
201
200202
201class MenuBase(UserAttributeCache):203class MenuBase(UserAttributeCache):
202 """Base class for facets and menus."""204 """Base class for facets and menus."""
@@ -220,13 +222,33 @@
220 """Override this in subclasses to do initialization."""222 """Override this in subclasses to do initialization."""
221 pass223 pass
222224
223 def _get_link(self, name):225 def _buildLink(self, name):
224 method = getattr(self, name)226 method = getattr(self, name)
225 linkdata = method()227 linkdata = method()
226 # The link need only provide ILinkData. We need an ILink so that228 # The link need only provide ILinkData. We need an ILink so that
227 # we can set attributes on it like 'name' and 'url' and 'linked'.229 # we can set attributes on it like 'name' and 'url' and 'linked'.
228 return ILink(linkdata)230 return ILink(linkdata)
229231
232 def _get_link(self, name):
233 request = get_current_browser_request()
234 if request is not None:
235 # XXX: I think we must not use a weak ref here because if we do so
236 # and templates do stuff like "context/menu:bugs/foo", then there
237 # would be no reference to the Link object, which would allow it
238 # to be garbage collected during the course of the request. On the
239 # other hand, I'm not 100% sure it's ok to use a dict in
240 # request.annotations.
241 cache = request.annotations.setdefault(
242 #MENU_ANNOTATION_KEY, WeakValueDictionary())
243 MENU_ANNOTATION_KEY, {})
244 key = (self._baseclassname, name)
245 link = cache.get(key)
246 if link is None:
247 link = self._buildLink(name)
248 cache[key] = link
249 return link
250 return self._buildLink(name)
251
230 def _rootUrlForSite(self, site):252 def _rootUrlForSite(self, site):
231 """Return the root URL for the given site."""253 """Return the root URL for the given site."""
232 try:254 try:
233255
=== added file 'lib/canonical/launchpad/webapp/tests/test_menu.py'
--- lib/canonical/launchpad/webapp/tests/test_menu.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/webapp/tests/test_menu.py 2009-08-03 20:38:34 +0000
@@ -0,0 +1,70 @@
1# Copyright 2009 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4__metaclass__ = type
5
6import unittest
7
8from zope.security.management import newInteraction
9
10from canonical.launchpad.webapp.menu import (
11 Link, MENU_ANNOTATION_KEY, MenuBase)
12from canonical.launchpad.webapp.publisher import get_current_browser_request
13from canonical.testing import DatabaseFunctionalLayer
14from lp.testing import ANONYMOUS, login, logout, TestCase
15
16
17class TestMenu(MenuBase):
18 links = ['test_link']
19 _baseclassname = 'TestMenu'
20 times_called = 0
21
22 def test_link(self):
23 self.times_called += 1
24 return Link('+test', 'Test', summary='Summary')
25
26
27class TestMenuBaseLinkCaching(TestCase):
28 """Link objects generated by MenuBase subclasses are cached.
29
30 They are cached in the request as their state can't change during the
31 lifetime of a request, but we cache them because some of them are
32 expensive to generate and there are plenty of pages where we use
33 "context/menu:bugs/foo" in TAL more than once, which causes the whole list
34 of Links for the Bugs facet to be re-generated every time.
35 """
36 layer = DatabaseFunctionalLayer
37
38 def tearDown(self):
39 logout()
40
41 def test_no_cache_when_there_is_no_request(self):
42 # Calling login() would cause a new interaction to be setup with a
43 # LaunchpadTestRequest, so we need to call newInteraction() manually
44 # here.
45 newInteraction()
46 menu = TestMenu(object())
47 menu._get_link('test_link')
48 self.assertEquals(menu.times_called, 1)
49 menu._get_link('test_link')
50 self.assertEquals(menu.times_called, 2)
51
52 def test_cache_when_there_is_a_request(self):
53 login(ANONYMOUS)
54 menu = TestMenu(object())
55 menu._get_link('test_link')
56 self.assertEquals(menu.times_called, 1)
57 menu._get_link('test_link')
58 self.assertEquals(menu.times_called, 1)
59
60 def test_correct_value_is_cached(self):
61 login(ANONYMOUS)
62 menu = TestMenu(object())
63 link = menu._get_link('test_link')
64 request = get_current_browser_request()
65 cache = request.annotations.get(MENU_ANNOTATION_KEY)
66 self.assertEquals([link], cache.values())
67
68
69def test_suite():
70 return unittest.TestLoader().loadTestsFromName(__name__)
071