Greatly optimize the review request and diff viewer pages.

 






 

 


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

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-1.6.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
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





   

   







  

 



 

  


   

   

    
 
medanat


    
   


  

  

  



    




  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. 


 
    


    

 
    

    2. 

















  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!
      
 
      

      2. 


 
    


    

 
    

    3. 






  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.
      
 
      

      2. 


 
    


    

 
    

    4. 






  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.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Alphabetical order, unless these imports follow a different order.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/models.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This doesn't seem right.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   





 ME





   

   







  

 



 

  


   

   

    
 
medanat


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...