Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/parser.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/parser.py

Col: 38
 E261 at least two spaces before inline comment

Col: 39
 E262 inline comment should start with '# '

Col: 9
 E265 block comment should start with '# '

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/parser.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/parser.py

Perhaps we should use BytesIO?

I believe this will result in an extra newline. How about:

file.data += b'\n'.join(buf) + '\n'

Same comment here with the extra newline.

This isn't necessary, since we're done with the buffer.

Style nit: blank line between these.

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/parser.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/parser.py

Hey Michael,
This looks like a great improvement! We've been writing new code to use this same method, but the diff parser is much older, and we weren't aware of this performance issue before.
I'll need to take some time to dig into this (the change is small, but I'll want to do some additional testing anyway). This will be great to get in, though! Thanks!

Hey, thanks again for the contribution. Your e-mail just now reminded me of it (been busy with other things, as usual).
The fundamental idea to get rid of string concatenation and toward list processing is probably the right move. String concatenation in Python is actually faster than appending to lists and then joining these days, but there is the memory requirement, as you note. By storing in a list and then joining, we can reuse the strings coming in and concat once at the end, saving a good deal of memory.
However, the way it's being done here only solves a part of the problem (other SCMTools, like Git, have custom diff parsers that also manipulate file.data and aren't receiving any benefit here and in fact likely negate the performance benefits, if they don't flat-out generate incorrect results). There are other concerns I have about assumptions being made with regard to the file contents (I'll cover this below), and API breakage (parse_diff_line's signature changes, which can break subclasses).
For the performance gains to be optimal and the results to be correct, the following must be true:

Data must always goes in the list, and we must never concatenate to file.data until we're finished.
Data from self.lines should always go into the list as-is, unless we're modifying the line in question. By retaining the exact string, we take advantage of the ref counting of the string, saving memory.
Since subclasses can freely override parts of the diff parsing, this function cannot safely assume that the joined list (b'\n'.join(buf)) can be appending to file.data at the places where it's assuming this now.

The last one is a problem. Right now, what's happening is you're building this buffer and assuming at the end that you have a complete picture of the state of the diff, later appending its contents. However, subclasses are also going to assume they can modify file.data, and will do so before you append anything, causing lines to be out of order. This would happen with Git if it didn't override parse (which also means it's not getting any performance benefits here).
So to fix all this without having to rewrite and break the world, I suggest the following:

Introduce a new object, let's call it DiffBytes, that acts like a string (can be concatenated using +=) but wraps appending to a list. Something like:


class DiffBytes(object):
    def __init__(self):
        self._buf = []

    def __iadd__(self, s):
        self._buf.append(s)
        return self

    def __str__(self):
        return ''.join(self._buf)



Set file.data to this initially, and normalize back to strings when parsing has completed for the file.

What the code is all doing today, adding more data, is fine, but string concatenation in Python isn't very optimal. Lists and buffers are, but we can't change every call without breaking API compatibility (and can't change function signatures for the same reasons). So instead, how about taking data and making it something other than a string.
When joining, don't put newlines in-between. Just do ''.join(buf). Leave newlines to the code doing the += (or you'll break code assuming it can build strings piece-meal).
For those callers doing a += on a line coming in from self.lines, split those into two calls: One for the line (so we can reuse an existing string in memory), and one for the +=.

This should give you the same speed benefits that your change has for all SCMs but Git, and introduce those benefits to Git, and any other subclasses modifying data themselves. It should also keep the API stable.
One thing I noted in the review request is that there was no mention of the unit tests passing. That's going to also be important in order for us to accept the change. Also, explicit testing with Git.