flint_table.cc uses zlib inefficiently

[tlipcon]

FlintTable reallocates the zlib stream object for every read and write. This is inefficient as it causes a lot of small mallocs, which translates to a lot of mmamps, brks, and munmaps.

This patch fixes the issue and speeds up "make flint-check" by about 20% on my machine. It speeds up indexing of a large database by a large factor and reduces system CPU percent from around 40% to around 2%.

[richard]

I've done some profiling of this patch. On my system (ubuntu hardy, glibc version 2.7-10ubuntu4, zlib version 1:1.2.3.3.dfsg-7ubuntu1), it makes little difference, though testing under strace does reveal that it reduces the number of brk() calls per document add from an average of about 3 to 1. The original poster mentioned on IRC that using tcmalloc instead of the default alloc on his system (debian etch, but also tested with debian lenny with similar results).

I think it's probably worth including this patch on trunk, though it needs a little cleaning up: in particular, the destructor doesn't clean up the lazily allocated structures, so I believe there's a memory leak with the patch. The original poster also mentioned on IRC that the FlintTable destructor isn't virtual, which it probably should be, but I've not had time to look into this so far.

[richard]

Improved patch

[richard]

Attached an improved patch which:

• tidies formatting
• makes the zlib objects mutable, so the inflate one can be initialised lazily too.
• avoids raising exceptions from destructors.

Would appreciate review from Olly, but I think this is good to go, so marking it for 1.1.0. It might be worth backporting to 1.0.x, too.

[richard]

Improved patch - don't call lazy_alloc from FlintTable::FlintTable

[richard]

Backport of my fixed-up patch to 1.0 branch

[richard]

We've just tested the patch on the 1.0 branch (with flint); search speeds don't seem to be altered at all by it; indexing speeds are possibly slightly improved, but the difference is likely within the error margin:

146s to index and commit 10k docs with unpatched xapian 144.8s for patched

[olly]

A virtual destructor is only required if you ever delete an object of a derived class via a pointer to the base class, which we never do in this case (in fact we never allocate FlintTable on the heap).

I'll take a look at the patch shortly, but it does sound worthwhile at least conceptually.

[olly]

Assigning to Richard as he seems to be working actively on this.

Comments on the latest patch (I only looked at the trunk one, but we should certainly consider backporting if this looks good on trunk as it sounds like it can make quite a difference on some platforms):

Don't forget to thank Todd in AUTHORS!

As above, the destructor doesn't need to be virtual.

I think this comment is still helpful and should be kept:

            // Deflate failed - presumably the data wasn't compressible.

I wouldn't worry about errors from inflateEnd() and deflateEnd() - we already check for problems which matter at the time and in the current code an error in either calls abort() (or is ignored when expected). I'd say just cast the return values to (void) to make it clear this is deliberate.

The two new methods really ought to have doc comments, and the new zstream pointer members probably should get one each.

I wonder if the laziness is worthwhile - we could just initialise these for a compressed table up front. They could even be members rather than pointers, but that's a fair overhead for a non-compressed table. I don't have a problem with the approach here though - just wondering aloud really...

[richard]

Applied a slightly modified version of Refactor-zlib-usage-in-FlintTable-and-ChertTable?-to-avoid-reallocat.3.patch to head (in r12018). Differences were:

• Add Todd to AUTHORS.
• don't make the destructors virtual.
• Put back the "Deflate failed" comment.
• ignore errors from inflateEnd() and deflateEnd() (with cast to void)

Then realised I forgot to update the doccomments, so added them in r12019.

I think the laziness is worthwhile - in particular, if you don't write to the table, it means that the deflate zstream never gets created, which is nice.

Marking this as fixed - it could probably usefully be backported to 1.0.x, but this will get done in the normal course of going through 1.1 checking for backport candidates.

[olly]

Sorry, going to reopen this one, at least briefly. Having had a look at the patch as applied:

• You only updated chert, and not flint, for the last of your bullet points (which I've addressed in r12020).

• Errors from inflateReset() and deflateReset() are ignored. Not sure what the best response is. We could just throw an error, but perhaps more robust would be to call inflateEnd() and then delete the zstream, then drop through to the allocating code?

• If inflateInit2() or deflateInit2() fails, should we delete the structure? The API docs for zlib don't go into detail on this, but particularly if the error was Z_MEM_ERROR it seems likely that the structure isn't fully set up and the docs for the reset methods say "does not free and reallocate all the internal compression state".

Sorry for not noticing the latter two before.

I might take a look at the zlib source later to see if that sheds light on these. If not actually explicitly documented, the behaviour is perhaps subject to change, but if it shows we do need to do something on Z_MEM_ERROR, then it means we definitely do.

[richard]

Looking at the code in zlib1g for inflateReset() and deflateReset(), they currently only return errors if given an invalid stream. For inflateReset():

    if (strm == Z_NULL || strm->state == Z_NULL) return Z_STREAM_ERROR;

For deflateReset():

    if (strm == Z_NULL || strm->state == Z_NULL ||
        strm->zalloc == (alloc_func)0 || strm->zfree == (free_func)0) {
        return Z_STREAM_ERROR;
    }

This is only going to happen if a previous error occurred, or we failed to call the initialisation code somehow, so I think just raising an exception is perfectly reasonable here.

Looking at the code for inflateInit2() and deflateInit2 - they seem to carefully ensure that they clean up the structure on error, so all we should need to do is delete the zstream (which happens in our destructor), so I think we're okay there. On the other hand, inflateEnd() and deflateEnd() are safe to call on the resulting structure too (since the internal state pointer gets reset to Z_NULL) - this will result in a Z_STREAM_ERROR, but we can safely ignore this.

So, the current zlib implementation that I'm using is fine as is, and fine to add a call inflateEnd() or deflateEnd() if the init functions fail, but the real question is what other implementations do...

[richard]

I've added (in r12050) checks to throw an exception if the Reset() functions return an error code. My feeling is that we're fine to leave the handling of errors in the Init2() functions as it is, but feel free to add a handler if you like.

Passing this ticket over to you. ;-)

[olly]

Fixed in r12056.

[olly]

And removed a needless double initialisation in r12057.

[olly]

Backported for 1.0.11, r12058.