Speed up cPickle's internal output machinery - Code Review (original) (raw)

Can't Edit Can't Publish+Mail Start Review Created: 16 years, 1 month ago by Collin Winter Modified: 16 years, 1 month ago Reviewers: Antoine Pitrou, Alexandre Vassalotti Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/ Visibility: Public.	Patch Set 1# Patch Set 2 : Remove unneeded changes# Total comments: 11 Created: 16 years, 1 month ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+193 lines, -410 lines) Patch M Modules/cPickle.c View 1 68 chunks +193 lines, -410 lines 11 comments Download Messages Total messages: 3 Expand All Messages | Collapse All Messages Antoine Pitrou 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 ... 16 years, 1 month ago (2009-04-04 11:46:24 UTC)#1 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? Sign in to reply to this message. Alexandre Vassalotti Apart the minor issues I saw, I think the patch is good. Just add a ... 16 years, 1 month ago (2009-04-05 08:58:15 UTC)#2 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. Sign in to reply to this message. **Antoine Pitrou** 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 ... 16 years, 1 month ago (2009-04-05 15:20:02 UTC)#3 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. Sign in to reply to this message. Expand All Messages	Collapse All Messages

Issue 33070: Speed up cPickle's internal output machinery
Created 16 years, 1 month ago by Collin Winter
Modified 16 years, 1 month ago
Reviewers: Antoine Pitrou, Alexandre Vassalotti
Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/
Comments: 11