Code review comment for lp:~salgado/launchpad/cached-menus

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)

« Back to merge proposal