Greatly optimize the review request and diff viewer pages.

Review Request #3160 — Created June 25, 2012 and submitted

Information

Review Board
release-1.6.x

Reviewers

The review request page and diff viewer pages have been producing quite a large
number of queries, and this count goes up as the review/comment counts go up.

This change precomputes a lot of data for the review request and diff viewer pages
in order to reduce those queries. Some things that were previously computed as
individual queries per-review are now filtered and categorized out of single
queries up-front.

Some models, like BaseComment and Review, now have accessors that wrap access to
related objects. By default, these are equivalent to accessing the object directly,
but can be short-circuited by setting special variables on the model, which the
review request and diff viewer pages do.

There were also many places in templates where we only needed the ID of a related
object, but we'd still access the entire object just to get it. Now we use the IDs
directly.

With this change, a small review request has gone from 60 queries (total render time of
506ms) to 24 (180ms). A large one went from 396 queries (3.1s) to 24 (348ms)

The diff viewer on the large change went from 270 queries (2.8s) to 24 (330ms).
Tested with several review requests with lots of moving parts, and even compared HTML output.

See the timing above for examples.

We'll need to test this on reviews.reviewboard.org a fair bit before we release, just to make
sure there isn't any fallout.
Description From Last Updated

Tiny optimization, for both functions: def func(self): review_request = self.review_request for x in self.xs.all(): x._review_request = review_request I also tried …

ME medanat

The placement of "is_reply.boolean = True" seems weird or out of place. Also, shouldn't it be: is_reply = self.is_reply() or …

ME medanat

Why are you asserting False here?

ME medanat

This doesn't seem right.

daviddavid

Alphabetical order, unless these imports follow a different order.

ME medanat
ME
  1. Quick review. This seems like a really great optimization!
    
    One question, I am wondering if this would bump the RAM requirements for RB.
    1. It shouldn't. There are some caching tables we create, but we otherwise are storing all these objects in memory anyway. Sometimes multiple times, it turns out. So, I think while we're adding to the memory requirements by adding mapping tables and keeping some more objects in memory a little bit longer, we're saving a whole bunch by not having as many copies of the same objects all over the place from all the various queries and property accesses.
      
      For example, we always have the review request loaded. But then we access that review request (but a different instance) through other objects. And same with the reviews.
      
      It'd be interesting to see the memory footprint used before and after. Not quite sure how to reliably get that.
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Tiny optimization, for both functions:
    
    def func(self):
    
        review_request = self.review_request
    
        for x in self.xs.all():
            x._review_request = review_request
    
    I also tried using a map for these functions, something like:
    
    map(lambda x: setattr(x, '_review_request', review_request), all)
    
    but after some quick tests, the for loop still performs better.
    1. Good optimization. Thanks!
  3. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    The placement of "is_reply.boolean = True" seems weird or out of place.
    
    Also, shouldn't it be:
    
    is_reply = self.is_reply() or something of the sort?
    1. This looks a bit strange, but I assure you it's there for a reason :)
      
      What we're doing is setting a property on the is_reply function. Functions can, like objects, contain properties. In this case, Django will look for the "boolean" property when rendering a column for this in the database UI. If "boolean" is set, it will display a graphical indicator to differentiate true vs. false.
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    Why are you asserting False here?
    1. Oops, that wasn't supposed to be there. I added it for debugging to make sure I was going down the correct codepath.
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Alphabetical order, unless these imports follow a different order.
  6. 
      
david
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    This doesn't seem right.
  3. 
      
chipx86
david
  1. Ship It!
  2. 
      
ME
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed