253: diff view hangs for more than 100 files

sixten******@gmai***** (Google Code) (Is this you? Claim this profile.)
April 20, 2008 
when the diff contains 101 or more modified files the diff view never
returns with any output.

when creating a diff that contains exactly 100 modified files the diff view
just works fine. within a few seconds the output is generated. 

my installed system:
revision log: 964
chipx86
#1 chipx86 
It's a coincidence that it's 101+. I would say just don't do this, as there really
isn't a great fix, but I'll keep it open in case we somehow find a fix for this.
  • +Component-DiffViewer
chipx86
#2 chipx86
  • -Priority-Medium
    +Priority-Low
#3 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
this is the very first problem I hit...
#4 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
this is coming up quite often in our environment, to the point that I've been asked
to write a script that automatically breaks diffs up :(

do you understand exactly what the problem is?  perhaps I could take a look at fixing it.
chipx86
#5 chipx86 
I believe it's just that it takes a long time to parse (we could definitely optimize
better) and a while to generate the template (doing interline diffs and all),
especially if you have syntax highlighting on. Another large part of it is that the
browser has a hard time rendering so much content on some computers (the DOM gets to
be quite large).

I had a workaround in mind for such cases where, when the diffset is 20+ files, we
collapse all the diffs after the first 10 or whatever and allow for expanding them
manually. When expanded, we query the server for the diff of that file only, so that
we're not doing this for all the diffs in one go. Another option is to paginate the
diff viewer.
  • -Priority-Low
    +Priority-High
#6 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Yeah, that makes sense.  Even if reviewboard were super efficient, the browser would
eventually fall over, so sooner or later we'll need to be able to break up the diffs.

I'll take a look at some kind of collapsing or pagination.  But my django & python
skillz aren't super hot.  Thanks.
#7 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
I've attached a patch that adds pagination to the diff viewer.  I cribbed most of the
pagination stuff right from the django docs.
  • +
    diff-pagination.patch 
    Index: settings.py
    ===================================================================
    --- settings.py	(revision 1153)
    +++ settings.py	(working copy)
    @@ -166,3 +166,8 @@
         dependency_error('Unable to read settings_local.py.')
     
     TEMPLATE_DEBUG = DEBUG
    +
    +# the number of files to display per page in the diff viewer
    +PAGINATE_DIFFS_BY = 10
    +# the number of extra files required before adding another page
    +PAGINATE_ORPHANS = 5
    Index: templates/diffviewer/changeindex.html
    ===================================================================
    --- templates/diffviewer/changeindex.html	(revision 1153)
    +++ templates/diffviewer/changeindex.html	(working copy)
    @@ -21,3 +21,15 @@
      {% endif %}
     {% endfor %}
     </ol>
    +{% if is_paginated %}
    + This diff has been split across {{ pages }} pages:
    + {% if has_previous %}<span class="paginate-previous"><a href="?page={{ previous_page }}" title="Previous Page">&lt;</a></span>{% endif %}
    + {% for num in page_numbers %}
    +  {% ifequal num page %}
    +   <span clas
david
#8 david 
Can you put this up on http://reviews.review-board.org/ for discussion?
#9 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Hm, good idea, but I can't manage to do it.  I get the error message
"'UploadDiffForm' object has no attribute 'cleaned_data'"

I left the base diff path empty, and selected the review board svn repository.  Is
that right?
chipx86
#10 chipx86 
Assuming you generated the diff in the base reviewboard directory, you should specify
"/trunk" for the base diff path.
#11 tim****@gmai***** (Google Code) (Is this you? Claim this profile.) 
Gotcha.  The review request is here: http://reviews.review-board.org/r/265/

Unfortunately, paginating the viewer isn't quite enough in our environment.  People
are submitting 5 megabyte diff files from large merges.

What is reviewboard calculating when it brings up the diff viewer?  Could it be
precalculated and saved once?
david
#12 david 
The diff viewer could be optimized more, but now it's paginated, so this particular
bug is fixed.  Thanks.
  • +Fixed