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
1=== modified file 'lib/canonical/launchpad/webapp/menu.py'
2--- lib/canonical/launchpad/webapp/menu.py 2009-07-17 00:26:05 +0000
3+++ lib/canonical/launchpad/webapp/menu.py 2009-08-03 20:38:34 +0000
4@@ -197,6 +197,8 @@
5 # Marker object that means 'all links are to be enabled'.
6 ALL_LINKS = object()
7
8+MENU_ANNOTATION_KEY = 'canonical.launchpad.webapp.menu.links'
9+
10
11 class MenuBase(UserAttributeCache):
12 """Base class for facets and menus."""
13@@ -220,13 +222,33 @@
14 """Override this in subclasses to do initialization."""
15 pass
16
17- def _get_link(self, name):
18+ def _buildLink(self, name):
19 method = getattr(self, name)
20 linkdata = method()
21 # The link need only provide ILinkData. We need an ILink so that
22 # we can set attributes on it like 'name' and 'url' and 'linked'.
23 return ILink(linkdata)
24
25+ def _get_link(self, name):
26+ request = get_current_browser_request()
27+ if request is not None:
28+ # XXX: I think we must not use a weak ref here because if we do so
29+ # and templates do stuff like "context/menu:bugs/foo", then there
30+ # would be no reference to the Link object, which would allow it
31+ # to be garbage collected during the course of the request. On the
32+ # other hand, I'm not 100% sure it's ok to use a dict in
33+ # request.annotations.
34+ cache = request.annotations.setdefault(
35+ #MENU_ANNOTATION_KEY, WeakValueDictionary())
36+ MENU_ANNOTATION_KEY, {})
37+ key = (self._baseclassname, name)
38+ link = cache.get(key)
39+ if link is None:
40+ link = self._buildLink(name)
41+ cache[key] = link
42+ return link
43+ return self._buildLink(name)
44+
45 def _rootUrlForSite(self, site):
46 """Return the root URL for the given site."""
47 try:
48
49=== added file 'lib/canonical/launchpad/webapp/tests/test_menu.py'
50--- lib/canonical/launchpad/webapp/tests/test_menu.py 1970-01-01 00:00:00 +0000
51+++ lib/canonical/launchpad/webapp/tests/test_menu.py 2009-08-03 20:38:34 +0000
52@@ -0,0 +1,70 @@
53+# Copyright 2009 Canonical Ltd. This software is licensed under the
54+# GNU Affero General Public License version 3 (see the file LICENSE).
55+
56+__metaclass__ = type
57+
58+import unittest
59+
60+from zope.security.management import newInteraction
61+
62+from canonical.launchpad.webapp.menu import (
63+ Link, MENU_ANNOTATION_KEY, MenuBase)
64+from canonical.launchpad.webapp.publisher import get_current_browser_request
65+from canonical.testing import DatabaseFunctionalLayer
66+from lp.testing import ANONYMOUS, login, logout, TestCase
67+
68+
69+class TestMenu(MenuBase):
70+ links = ['test_link']
71+ _baseclassname = 'TestMenu'
72+ times_called = 0
73+
74+ def test_link(self):
75+ self.times_called += 1
76+ return Link('+test', 'Test', summary='Summary')
77+
78+
79+class TestMenuBaseLinkCaching(TestCase):
80+ """Link objects generated by MenuBase subclasses are cached.
81+
82+ They are cached in the request as their state can't change during the
83+ lifetime of a request, but we cache them because some of them are
84+ expensive to generate and there are plenty of pages where we use
85+ "context/menu:bugs/foo" in TAL more than once, which causes the whole list
86+ of Links for the Bugs facet to be re-generated every time.
87+ """
88+ layer = DatabaseFunctionalLayer
89+
90+ def tearDown(self):
91+ logout()
92+
93+ def test_no_cache_when_there_is_no_request(self):
94+ # Calling login() would cause a new interaction to be setup with a
95+ # LaunchpadTestRequest, so we need to call newInteraction() manually
96+ # here.
97+ newInteraction()
98+ menu = TestMenu(object())
99+ menu._get_link('test_link')
100+ self.assertEquals(menu.times_called, 1)
101+ menu._get_link('test_link')
102+ self.assertEquals(menu.times_called, 2)
103+
104+ def test_cache_when_there_is_a_request(self):
105+ login(ANONYMOUS)
106+ menu = TestMenu(object())
107+ menu._get_link('test_link')
108+ self.assertEquals(menu.times_called, 1)
109+ menu._get_link('test_link')
110+ self.assertEquals(menu.times_called, 1)
111+
112+ def test_correct_value_is_cached(self):
113+ login(ANONYMOUS)
114+ menu = TestMenu(object())
115+ link = menu._get_link('test_link')
116+ request = get_current_browser_request()
117+ cache = request.annotations.get(MENU_ANNOTATION_KEY)
118+ self.assertEquals([link], cache.values())
119+
120+
121+def test_suite():
122+ return unittest.TestLoader().loadTestsFromName(__name__)
123