Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#325 closed enhancement (fixed)

flint_table.cc uses zlib inefficiently

Reported by: tlipcon Owned by: olly
Priority: normal Milestone: 1.0.11
Component: Backend-Flint Version: SVN trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: All

Description

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%.

Attachments (5)

0001-Refactor-zlib-usage-in-FlintTable-to-avoid-reallocat.patch (9.5 KB) - added by tlipcon 11 years ago.
Refactor-zlib-usage-in-FlintTable-and-ChertTable-to-avoid-reallocat.patch (17.8 KB) - added by richard 11 years ago.
Refactor-zlib-usage-in-FlintTable-and-ChertTable-to-avoid-reallocat.2.patch (18.9 KB) - added by richard 11 years ago.
Improved patch
Refactor-zlib-usage-in-FlintTable-and-ChertTable-to-avoid-reallocat.3.patch (18.6 KB) - added by richard 11 years ago.
Improved patch - don't call lazy_alloc from FlintTable::FlintTable
Refactor-zlib-usage-in-FlintTable-1.0-branch-to-avoid-reallocat.patch (9.4 KB) - added by richard 11 years ago.
Backport of my fixed-up patch to 1.0 branch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 11 years ago by 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.

comment:2 Changed 11 years ago by richard

  • Milestone set to 1.1.0
  • Version set to SVN trunk

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.

Changed 11 years ago by richard

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

Changed 11 years ago by richard

Backport of my fixed-up patch to 1.0 branch

comment:3 Changed 11 years ago by 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

comment:4 Changed 11 years ago by 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.

comment:5 Changed 11 years ago by olly

  • Owner changed from olly to richard

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...

comment:6 Changed 11 years ago by richard

  • Status changed from new to assigned

comment:7 Changed 11 years ago by richard

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:8 Changed 11 years ago by olly

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:9 Changed 11 years ago by 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...

comment:10 Changed 11 years ago by richard

  • Owner changed from richard to olly
  • Status changed from reopened to new

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. ;-)

comment:11 Changed 11 years ago by olly

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in r12056.

comment:12 Changed 11 years ago by olly

And removed a needless double initialisation in r12057.

comment:13 Changed 11 years ago by olly

  • Milestone changed from 1.1.0 to 1.0.11

Backported for 1.0.11, r12058.

Note: See TracTickets for help on using tickets.