Add a more scalable version of diff fragment loading

Review Request #757 — Created March 9, 2009 and submitted

chipx86
Review Board SVN (deprecated)
trunk
reviewboard
Asynchronous diff fragment loading had a flaw in that it required one HTTP GET per comment. This didn't scale well when dealing with hundreds of comments, as it required hundreds of requests, bogging down the server and the browser. If the fragments at all fell out of the cache server-side, it would slow things down further.

We now batch loads. The old fragment URL is gone, replaced by a new one that cares only about a list of comment IDs, rather than needing a review ID or anything. This allows fragments to be retrieved in bulk, across any number of filediffs.

We take advantage of this now by storing lists of comments that belong on the same filediff or interfilediff. We can use this to batch loads such that if 50 comments were on file1 and 20 on file2, we would have two HTTP GETs, one loading all the comments on file1 and another loading the comments on file2.

This speeds up both the browser and diff loading in all cases.

Furthermore, instead of an AJAX call, we now append a script tag. This allows us to take advantage of browser-side caching properly.

This is implemented for both the review request page and the review dialog.
Tested on a variety of review requests that I have here. It works across files and across diffs, and there's a noticeable improvement in load time, even in the case of only a few comments, since they'll now appear at the same time instead of progressively on the same file.
david
  1. I think this is okay, but I'm not totally familiar with the javascript here. As long as you've tested this a lot, go for it.
  2. 
      
Loading...