Speed up cPickle's pickling generally

[collinwinter]

BPO	5683
Nosy	@pitrou, @avassalotti
Superseder	bpo-9410: Add Unladen Swallow's optimizations to Python 3's pickle.
Files	cpickle_writes.patch: Patch against trunk, r71111 cpickle_writes-r73872.patch: Patch against python27 trunk, r73872 cPickle.-r77393.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2010-08-05.08:17:01.552>
created_at = <Date 2009-04-04.00:02:17.156>
labels = ['extension-modules', 'performance']
title = "Speed up cPickle's pickling generally"
updated_at = <Date 2010-08-05.08:17:01.551>
user = 'https://bugs.python.org/collinwinter'

bugs.python.org fields:

activity = <Date 2010-08-05.08:17:01.551>
actor = 'alexandre.vassalotti'
assignee = 'none'
closed = True
closed_date = <Date 2010-08-05.08:17:01.552>
closer = 'alexandre.vassalotti'
components = ['Extension Modules']
creation = <Date 2009-04-04.00:02:17.156>
creator = 'collinwinter'
dependencies = []
files = ['13603', '14466', '15808']
hgrepos = []
issue_num = 5683
keywords = ['patch', 'needs review']
message_count = 18.0
messages = ['85349', '90185', '90212', '95521', '95576', '97484', '97514', '97515', '97516', '97517', '97518', '97520', '97561', '97704', '97705', '97711', '97712', '112957']
nosy_count = 4.0
nosy_names = ['collinwinter', 'pitrou', 'alexandre.vassalotti', 'jcsalterego']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '9410'
type = 'performance'
url = 'https://bugs.python.org/issue5683'
versions = ['Python 2.7', 'Python 3.2']

[collinwinter]

This patch simplifies cPickle's complicated internal buffering system.
The new version uses a single buffer closer to the Pickler object,
flushing to a file object only when necessary. This avoids the overhead
of several indirection layers for what are frequently very small writes.

Benchmarked against virgin trunk r71058 using "perf.py -r -b
pickle,pickle_list,pickle_dict":

pickle:
Min: 2.225 -> 1.962: 13.37% faster
Avg: 2.254 -> 1.994: 13.03% faster
Significant (t=70.763434, a=0.95)

pickle_dict:
Min: 2.097 -> 1.418: 47.83% faster
Avg: 2.136 -> 1.446: 47.75% faster
Significant (t=214.900146, a=0.95)

pickle_list:
Min: 1.128 -> 0.777: 45.22% faster
Avg: 1.152 -> 0.807: 42.65% faster
Significant (t=169.789433, a=0.95)

A similar patch for unpickling will follow. As bpo-5670 and 5671 are
committed, I'll update this patch to support them.

This patch passes all the tests added in bpo-5665. I would recommend
reviewing that patch first. I'll port to py3k once this is reviewed for
trunk. This patch is available on Rietveld at
http://codereview.appspot.com/33070.

[pitrou]

Any updates?

[jcsalterego]

Applied collinwinter's patch and svn up'd to r73872; replaced additional
instances of self->write_func with _Pickler_Write.

Passed test_xpickle.py as referenced by bpo-5665.

[pitrou]

Are you still willing to work on this?

[avassalotti]

Last august, I worked on integrating Collin's optimization work into
py3k in a local Mercurial branch. So, I can champion these changes into
py3k, if Collin is unavailable.

And if Collin allows me, I would like to merge the other pickle
optimizations currently in Unladen Swallow.

[smontanaro]

Updated the patch against the latest version of cPickle.c (r77393). All tests pass on my Mac.

[pitrou]

There were a couple of comments on the Rietveld code review above.

[smontanaro]

Antoine> There were a couple of comments on the Rietveld code review
Antoine> above.

Indeed there are. Given that the Unladen Swallow folks were focusing on the
2.6 branch and their goal was to improve performance I don't see any reason
to not accept what they've done, then tweak it for 2.7/3.1 assuming the
changes you and Alexandre suggested further improve performance.

Skip

[pitrou]

Indeed there are. Given that the Unladen Swallow folks were focusing on the
2.6 branch and their goal was to improve performance I don't see any reason
to not accept what they've done, then tweak it for 2.7/3.1 assuming the
changes you and Alexandre suggested further improve performance.

The main thing I'm worried about is the potentially unbounded buffering,
since it could reduce performance (or even thrash the machine) instead
of improving it.

[smontanaro]

Antoine> The main thing I'm worried about is the potentially unbounded
Antoine> buffering, since it could reduce performance (or even thrash
Antoine> the machine) instead of improving it.

Got a test case in mind? If so, I'll code it up and compare 2.6 and Unladen
Swallow as well as offer it up for inclusion in the U-S test suite.

S

[pitrou]

Got a test case in mind? If so, I'll code it up and compare 2.6 and Unladen
Swallow as well as offer it up for inclusion in the U-S test suite.

Trying to pickle a structure that's larger than half the RAM should do
the trick. Something like a list of large strings.

[pitrou]

Quick test on a 3GB machine:

Without patch ("top" shows the process reaches 1.2GB RAM max):

$ time ./python -c "import cPickle;l=['a'*1024 for i in xrange(1000000)];cPickle.dump(l, open('/dev/null', 'wb'))"
10.67user 1.47system 0:12.92elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+319042minor)pagefaults 0swaps

With the patch, the same command quickly swaps hopelessly and after 5 minutes of elapsed time I finally manage to kill the process.

[smontanaro]

Antoine> With the patch, the same command quickly swaps hopelessly and
Antoine> after 5 minutes of elapsed time I finally manage to kill the
Antoine> process.

Verified with an Unladen Swallow test case. I'll see if I can fix it.

S

[smontanaro]

You can fix it if you are dumping to a file, however if you are calling dumps() you are kind of screwed if dumping large objects. There's no place to flush the buffer.

I have a fix to Unladen Swallow's cPickle module. I'm run it by them before submitting it here.

[pitrou]

Do we need an intermediate buffer at all when called from dumps()? How about allocating the buffer as a PyStringObject, so that it can be used directly for the result in that case?
(IIRC there's a handy _PyString_Resize function)

[smontanaro]

Perhaps. Let's take it one step at a time though. If I change your
large pickle example to use dumps() instead of dump() in an unsullied
Python2.5 I get a MemoryError. In general, I think you have to be
careful using dumps(). Any attempt to solve that problem will likely
be independent of the Unladen Swallow patches.

[smontanaro]

Oh, BTW, the proposed fix is in Rietveld: http://codereview.appspot.com/189051

[avassalotti]

Too late to make this change in 2.x. And the patch in bpo-9410 includes the optimization for 3.x.