Storage and performance improvements

This patch contains storage and performance improvements to help scaling issues that I see with Chromium's Rietveld instance.  I originally thought they could use the same checksum to accomplish their task, which proved unworkable, however this explains why they ended up being together :)

Reuse base files when a copy already exists on the server.  This cuts down on upload time, and more importantly, on storage space (Chromium's instance is using 100MB/week!).

Don't keep recalculating the delta links each time an issue is shown.  On an issue with double digit patchsets and files, we see DeadlineExceededError often.  This caches the data once it's been calculated.  On one medium sized issue, it now takes 0.5s instead of 3s to generate the issue page.

[JohnA, 2008-10-01 04:41:45]

Whoever has time please have a look, thanks.

[Evan Martin, 2008-10-07 18:49:24]

http://codereview.appspot.com/6455/diff/1018/1021
File codereview/models.py (right):

http://codereview.appspot.com/6455/diff/1018/1021#newcode178
Line 178: checksum = db.TextProperty()
Could you comment whether this is a checksum of data or text?

http://codereview.appspot.com/6455/diff/1018/1021#newcode206
Line 206: deltas = db.StringProperty()
They have native support for storing lists.
http://code.google.com/appengine/docs/datastore/typesandpropertyclasses.html#...

http://codereview.appspot.com/6455/diff/1018/1019
File codereview/views.py (right):

http://codereview.appspot.com/6455/diff/1018/1019#newcode717
Line 717: if not file_info:
Why is this check necessary?

http://codereview.appspot.com/6455/diff/1018/1019#newcode1129
Line 1129: # issue to get things done, as opposed to an all-or-nothing approach.
Yikes, that makes me sad.  Good comment.

http://codereview.appspot.com/6455/diff/1018/1020
File static/upload.py (right):

http://codereview.appspot.com/6455/diff/1018/1020#newcode546
Line 546: ErrorExit("Got error status from %s:\n%s" % (command, data))
Oh man, this is gonna make my life much happier.

http://codereview.appspot.com/6455/diff/1018/1020#newcode666
Line 666: if file_id_str.find("nobase") != -1:
Could this be
  if file_id_str.startswith("nobase_"):
?

http://codereview.appspot.com/6455/diff/1018/1020#newcode668
Line 668: file_id_str = file_id_str[file_id_str.rfind("_") + 1:]
Is this trying to strip out the "nobase" bit?
You could do
file_id_str[len('nobase_'):]

[JohnA, 2008-10-07 21:15:39]

http://codereview.appspot.com/6455/diff/1018/1021
File codereview/models.py (right):

http://codereview.appspot.com/6455/diff/1018/1021#newcode178
Line 178: checksum = db.TextProperty()
On 2008/10/07 18:49:24, Evan Martin wrote:
> Could you comment whether this is a checksum of data or text?

This is a checksum on either, depending on the file type.

http://codereview.appspot.com/6455/diff/1018/1021#newcode206
Line 206: deltas = db.StringProperty()
On 2008/10/07 18:49:24, Evan Martin wrote:
> They have native support for storing lists.
>
http://code.google.com/appengine/docs/datastore/typesandpropertyclasses.html#...
> 

Right, but I still need a list of id pairs.  Instead of making yet another Model
entity (and putting up with the overhead), I packed them in one string.

http://codereview.appspot.com/6455/diff/1018/1019
File codereview/views.py (right):

http://codereview.appspot.com/6455/diff/1018/1019#newcode717
Line 717: if not file_info:
On 2008/10/07 18:49:24, Evan Martin wrote:
> Why is this check necessary?

Because splitting an empty string returns an array with one empty string:

>>> ''.split('|')
['']

http://codereview.appspot.com/6455/diff/1018/1020
File static/upload.py (right):

http://codereview.appspot.com/6455/diff/1018/1020#newcode666
Line 666: if file_id_str.find("nobase") != -1:
On 2008/10/07 18:49:24, Evan Martin wrote:
> Could this be
>   if file_id_str.startswith("nobase_"):
> ?

I wrote it like this on purpose so that we can add other flags in the future..

http://codereview.appspot.com/6455/diff/1018/1020#newcode668
Line 668: file_id_str = file_id_str[file_id_str.rfind("_") + 1:]
On 2008/10/07 18:49:24, Evan Martin wrote:
> Is this trying to strip out the "nobase" bit?
> You could do
> file_id_str[len('nobase_'):]

I'm trying to do it in a generic way so I can add other flags in the future.

[Andi Albrecht, 2008-10-08 04:51:57]

http://codereview.appspot.com/6455/diff/1018/1021
File codereview/models.py (right):

http://codereview.appspot.com/6455/diff/1018/1021#newcode206
Line 206: deltas = db.StringProperty()
I wonder if it isn't time to add a property to patchset to store the number? At
least there's a TODO item for deleting patch sets... Wouldn't it make things
easier to have such an property and just store patch keys in a ListProperty
here?

http://codereview.appspot.com/6455/diff/1018/1019
File codereview/views.py (right):

http://codereview.appspot.com/6455/diff/1018/1019#newcode756
Line 756: # be uploaded.  We mark this by prepending 'nobase' to the id.
hm, I think we need a single place to document our protocol... Currently hints
about how responses look like and how the payload of a request should look are
scattered across the sources and it's getting harder to know what actually
happens when upload.py gets busy ;-)

http://codereview.appspot.com/6455/diff/1018/1019#newcode1073
Line 1073: patch.text != opatch.text):
I wonder if we should ignore the prelude here. Sometimes only the prelude
changes but not the actual patch, e.g. when the diff for a new patchset is
created against a new revision. In that case a delta link is displayed but diff2
shows no (visible) changes.

http://codereview.appspot.com/6455/diff/1018/1019#newcode1119
Line 1119: if patch.deltas == None:
Great! A common way to have the deltas for a patch can be reused later for
things like delta links on sbs...

http://codereview.appspot.com/6455/diff/1018/1020
File static/upload.py (right):

http://codereview.appspot.com/6455/diff/1018/1020#newcode610
Line 610: A dictionary that maps from filename to GetBaseFile's tuple.
You should mention here that filenames are retrieved from the diff output by
looking for lines starting with "Index:" or "Property changes on:". GitVCS
already adds such a line, but a comment would be helpful when we decide to add
other VersionControlSystem implementations.

[JohnA, 2008-10-09 19:55:01]

http://codereview.appspot.com/6455/diff/1018/1021
File codereview/models.py (right):

http://codereview.appspot.com/6455/diff/1018/1021#newcode206
Line 206: deltas = db.StringProperty()
On 2008/10/08 04:51:59, andi.albrecht wrote:
> I wonder if it isn't time to add a property to patchset to store the number?
At
> least there's a TODO item for deleting patch sets... Wouldn't it make things
> easier to have such an property and just store patch keys in a ListProperty
> here?

I was able to reimplement this using a list of patchset IDs, and generating the
user visible patchset counter in the loop.  Note that I had to add another
boolean to figure out whether the list has been generated or not.

http://codereview.appspot.com/6455/diff/1018/1019
File codereview/views.py (right):

http://codereview.appspot.com/6455/diff/1018/1019#newcode756
Line 756: # be uploaded.  We mark this by prepending 'nobase' to the id.
On 2008/10/08 04:51:59, andi.albrecht wrote:
> hm, I think we need a single place to document our protocol... Currently hints
> about how responses look like and how the payload of a request should look are
> scattered across the sources and it's getting harder to know what actually
> happens when upload.py gets busy ;-)

Where do you propose we put this information?

http://codereview.appspot.com/6455/diff/1018/1019#newcode1073
Line 1073: patch.text != opatch.text):
On 2008/10/08 04:51:59, andi.albrecht wrote:
> I wonder if we should ignore the prelude here. Sometimes only the prelude
> changes but not the actual patch, e.g. when the diff for a new patchset is
> created against a new revision. In that case a delta link is displayed but
diff2
> shows no (visible) changes.

I agree we need to do more work here, at the least to fix the example that you
give.  However I think it's best to keep to do this separately, as it's
orthogonal to this patch.

http://codereview.appspot.com/6455/diff/1018/1020
File static/upload.py (right):

http://codereview.appspot.com/6455/diff/1018/1020#newcode610
Line 610: A dictionary that maps from filename to GetBaseFile's tuple.
On 2008/10/08 04:51:59, andi.albrecht wrote:
> You should mention here that filenames are retrieved from the diff output by
> looking for lines starting with "Index:" or "Property changes on:". GitVCS
> already adds such a line, but a comment would be helpful when we decide to add
> other VersionControlSystem implementations.

Done.

[Evan Martin, 2008-10-09 20:53:23]

This looks good to me (with the caveat that I don't really understand this
code).  Andi?

http://codereview.appspot.com/6455/diff/1018/1021
File codereview/models.py (right):

http://codereview.appspot.com/6455/diff/1018/1021#newcode178
Line 178: checksum = db.TextProperty()
On 2008/10/07 21:15:39, JohnA wrote:
> On 2008/10/07 18:49:24, Evan Martin wrote:
> > Could you comment whether this is a checksum of data or text?
> 
> This is a checksum on either, depending on the file type.

What I had meant was "could you add a comment indicating that?"  :)