Speed up cPickle's internal output machinery

[Antoine Pitrou, 2009-04-04 11:46:24]

http://codereview.appspot.com/33070/diff/3/4
File Modules/cPickle.c (left):

http://codereview.appspot.com/33070/diff/3/4#oldcode2863
Line 2863: return NULL;
Nice simplication there :)

http://codereview.appspot.com/33070/diff/3/4
File Modules/cPickle.c (right):

http://codereview.appspot.com/33070/diff/3/4#newcode348
Line 348: 
Instead of resizing the buffer, you could accumulate a list of PyString objects
and use _PyString_Join() at the end. It would avoid realloc() calls on a
potentially big chunk of data.

By the way, should their be a limit to the internal buffering? Say, only buffer
up to 64KB data and then flush it to disk.

http://codereview.appspot.com/33070/diff/3/4#newcode431
Line 431: self->max_output_len = (self->output_len + n) * 2;
There should be some kind of overflow checking here. What I often do is use
size_t instead of Py_ssize_t for the internal calculation, and compare the
result to PY_SSIZE_T_MAX.

(there are some issues with checking signed overflows and compiler
optimizations: see http://code.python.org/hg/trunk/rev/76d3ab28bb71 )

http://codereview.appspot.com/33070/diff/3/4#newcode440
Line 440: /* This is faster than memcpy for the char *s we're copying. */
Why is it faster than memcpy? Because n is small?

[Alexandre Vassalotti, 2009-04-05 08:58:15]

Apart the minor issues I saw, I think the patch is good. Just add a limit on the
internal buffer size and the patch will be ready to be committed.

By the way, most of your changes will not be necessary for Python 3.0. Do like
the simplification of Pickler_Write and the separation of the flush operation in
its own function. Your patch also gave me an optimization idea for _pickle in
Python 3.0 — i.e., re-implement pickle.dumps using unlimited internal buffering
of Pickler.

http://codereview.appspot.com/33070/diff/3/4
File Modules/cPickle.c (left):

http://codereview.appspot.com/33070/diff/3/4#oldcode226
Line 226: nbytes = (size_t)bigger * sizeof(PyObject *);
I agree that this is unlikely to overflow. :-)

http://codereview.appspot.com/33070/diff/3/4
File Modules/cPickle.c (right):

http://codereview.appspot.com/33070/diff/3/4#newcode176
Line 176: self->data = PyMem_NEW(PyObject *, self->size);
You probably should use PyMem_Malloc() and do the (PyObject *) manually, instead
of using the obsolete PyMem_NEW interface.

http://codereview.appspot.com/33070/diff/3/4#newcode348
Line 348: 
I kinda like the idea of Antoine of using _PyString_Join(). The only problem is
you could ends up with a very large string copy in memory. So, you would need to
keep track of the total length of the strings kept.

Also, I agree that there should be a limit on the buffer length when Pickler is
used with streams.

http://codereview.appspot.com/33070/diff/3/4#newcode467
Line 467: Py_DECREF(output);
I think:

   Py_DECREF(output);
   if (ret == NULL)
      return -1;
   Py_DECREF(ret);
   return 0;

would be at least as efficient and slightly less magic.

http://codereview.appspot.com/33070/diff/3/4#newcode2633
Line 2633: if (_Pickler_Write(self, NULL, 0) < 0)
What does this do? The original cPickle interprets NULL to mean flush the
buffer, but I don't think this is what you want.

[Antoine Pitrou, 2009-04-05 15:20:02]

http://codereview.appspot.com/33070/diff/3/4
File Modules/cPickle.c (right):

http://codereview.appspot.com/33070/diff/3/4#newcode348
Line 348: 
Thinking about it, building lots of small PyStrings could have a non-negligible
overhead, while the asymptotic complexity is O(N) in both cases. So I withdraw
my suggestion.

(limiting the buffer size would still be nice, though)

http://codereview.appspot.com/33070/diff/3/4#newcode452
Line 452: self->output_len);
What you could do to avoid the copy would be to create a memoryview object
pointing to the internal buffer instead.
However, it would only work in trunk and py3k (2.6 doesn't have it), and I'm not
sure it would be accepted by all file-like objects.
See
http://code.python.org/hg/branches/py3k/file/e42bcf320c14/Modules/_io/buffere...
if you want some example of doing it.