Optimize most pages, but especially the diff viewer and diff fragments.

Review Request #3219 — Created July 15, 2012 and submitted

Review Board
Optimize most pages, but especially the diff viewer and diff fragments.

This is another round of optimizations to reduce query counts on pages.

In general, most pages will now have at least 1 less query. Some will
have 2 or 3 fewer.

The default session backend caused one query per view that always looked
up the session in the database. This was one of the first queries
executed. However, Django has another backend, cached_db, that will
first attempt to look up the data in memcached, and if it doesn't find
it, it will fall back to the database and then populate that data in
memcached. It's much faster, and saves one query per page.

The @valid_prefs_required decorator looked up the user's profile using
get_or_create, meaning the profile never got cached for the user. This
resulted in an additional profile query later on in most cases. We now
use get_profile() optimistically, and fall back on logic to create it.
This affects many pages on the site.

The My Account page got similar optimizations to reduce query counts, as
did get_enable_highlighting (used in diff generation).

The big changes though are to the diff viewer/fragment code. In
view_diff_fragment, we had one query for the DiffSet, another for the
interdiff's DiffSet (if specified), another for the FileDiff, and
another resulting one later for the FileDiff's DiffSet (since it wasn't
cached). And that was just for the core diffviewer's view_diff_fragment.
The wrapper in reviews added another one for the DiffSet and one for the
interdiff's DiffSet.

This was cleaned up to only have two queries total. One to fetch both
DiffSets (or just one if not doing an interdiff) at once, and one to
fetch the FileDiff. We grab the correct objects out of the query and we
assign the diffset to the FileDiff so we don't look it up again later.
view_diff_fragment now also accepts IDs instead of objects, so the
reviews wrapper doesn't need to do a fetch and pass it off.

This is a savings of 3-5 queries per fragment.

The other savings is in _query_for_diff, which would call
review_request.diffset_history and get_draft(user). The former was
unnecessary, since all we needed was the ID, so we save a query by
referencing the ID and not grabbing the object itself.

The get_draft(user) was a bit redundant. Many of the call sites already
had a draft. Now it no longer makes its own call, and instead requires
that a draft be provided.

All told, we save quite a few queries now, and every page (and API call)
has at least 1 fewer query (assuming the session is in the cache).

Some highlights:

The diff viewer (minus subsequent fragments) went from 20 to 16 queries.

Individual diff fragments went from 14 to 8.

The My Account page went from 10 to 7.
Tested each of the pages pretty thoroughly. Diff viewer, individual
fragments, interdiffs, My Account, dashboard, Submitters, Groups,
APIs. I didn't see any regressions.

Unit tests all passed.
  1. Ship It!
  2. setup.py (Diff revision 1)
    Was this something that slipped through, or intentional?
    1. Intentional, mostly. Going to choose a version range and bump Djblets soon.
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.6.x (5a31dd1)