The attached patch adds another version of cPickle.c's batch_dict(),batch_dict_exact(), which is specialized for "type(x) is dict". Thisprovides a nice performance boost when pickling objects that usedictionaries:Pickle:Min: 2.216 -> 1.858: 19.24% fasterAvg: 2.238 -> 1.889: 18.50% fasterSignificant (t=106.874099, a=0.95)Benchmark is athttp://code.google.com/p/unladen-swallow/source/browse/tests/performance/macro_pickle.py(driver is  ../perf.py; perf.py was run with "--rigorous -b pickle").This patch passes all the tests added inissue 5665. I would recommendreviewing that patch first. I'll port to py3k once this is reviewed fortrunk.

Without taking a very detailed look, the patch looks good.Are there already tests for pickling of dict subclasses? Otherwise, theyshould be added.

By the way, could the same approach be applied to lists and sets as well?

On Thu, Apr 2, 2009 at 12:20 PM, Antoine Pitrou <report@bugs.python.org> wrote:>> Antoine Pitrou <pitrou@free.fr> added the comment:>> By the way, could the same approach be applied to lists and sets as well?Certainly; seehttp://bugs.python.org/issue5671 for the list version.It doesn't make as big an impact on the benchmark, though.

> Certainly; seehttp://bugs.python.org/issue5671 for the list version.> It doesn't make as big an impact on the benchmark, though.How about splitting the benchmark in parts:- (un)pickling lists- (un)pickling dicts- (un)pickling sets(etc.)

Antoine: pickletester.py:test_newobj_generic() appears to test dictsubclasses, though in a roundabout-ish way. I don't know of any testsfor dict subclasses in the C level sense (ie, PyDict_Check() vsPyDict_CheckExact()). I can add more explicit tests for Python-leveldict subclasses, if you want.

The patch produces different output for an empty dict: a sequence "MARK SETITEMS" is written, which is useless and wastes 2 bytes.

> Antoine: pickletester.py:test_newobj_generic() appears to test dict> subclasses, though in a roundabout-ish way. I don't know of any tests> for dict subclasses in the C level sense (ie, PyDict_Check() vs> PyDict_CheckExact()). I can add more explicit tests for Python-level> dict subclasses, if you want.Well, Python-level dict subclasses are also C-level subclasses (in thePyDict_Check() sense), or am I mistaken?

I ported the patch to py3k. In addition, I added a special-case when thedict contains only one item; you probably want this special-case in thetrunk version as well.

Oops, I forgot to add the comment on top of batch_dict_exact in thepatch. Here is a better patch.

Oops again, I just remarked that the comment for batch_dict_exact refersto batch_dict as being above, but I copied batch_dict_exact beforebatch_dict. Here's a good patch (hopefully) that puts batch_dict_exactat the right place.

Silly me, I had changed the PyDict_Size call in outer loop for Py_SIZEand this is of course totally wrong. Here's a good patch (I am prettysure now! ;-) I ran the whole test suite and I saw no failures.Collin, you can go ahead and commit both patches. Nice work!

Sigh... silly me again. There is some other junk in my last patch.

FYI, I just added a pickle_dict microbenchmark to perf.py. Using thisnew microbenchmark, I see these results (perf.py -r -b pickle_dict):pickle_dict:Min: 2.092 -> 1.341: 56.04% fasterAvg: 2.126 -> 1.360: 56.37% fasterSignificant (t=216.895643, a=0.95)I still need to address the comment about pickling empty dicts.

Amaury, I can't reproduce the issue you're seeing with empty dicts.Here's what I'm doing:dhcp-172-19-19-199:trunk collinwinter$ ./python.exe Python 2.7a0 (trunk:71100M, Apr  3 2009, 14:40:49) [GCC 4.0.1 (Apple Inc. build 5490)] on darwinType "help", "copyright", "credits" or "license" for more information.>>> import cPickle, pickletools>>> data = cPickle.dumps({}, protocol=2)>>> pickletools.dis(data)    0: \x80 PROTO      2    2: }    EMPTY_DICT    3: .    STOPhighest protocol among opcodes = 2>>> data'\x80\x02}.'>>>What are you doing to produce the MARK SETITEMS sequence?

Sorry, I was wrong. I think I noticed that the case size==1 was handled differently, and incorrectly inferred the same for size==0.(btw, the patch for trunk was not updated)

Can this patch be used or ported to 2.5.x?

Sorry, it won't even be integrated in 2.6 actually. It's a new feature,not a bug fix.

Fixed the len(d) == 1 size regression. Final performance of the patchrelative to trunk:Using Unladen Swallow's perf.py -b pickle,pickle_dict on trunk:pickle:Min: 2.238 -> 1.895: 18.08% fasterAvg: 2.241 -> 1.898: 18.04% fasterSignificant (t=282.066701, a=0.95)pickle_dict:Min: 2.163 -> 1.375: 57.36% fasterAvg: 2.168 -> 1.376: 57.50% fasterSignificant (t=527.668441, a=0.95)Performance for py3k:pickle:Min: 2.849 -> 2.790: 2.10% fasterAvg: 2.854 -> 2.796: 2.09% fasterSignificant (t=27.624303, a=0.95)pickle_dict:Min: 2.121 -> 1.512: 40.27% fasterAvg: 2.128 -> 1.519: 40.13% fasterSignificant (t=283.406572, a=0.95)regrtest.py -uall test_xpickle passes all backwards-compatibility testsfor trunk, and all other tests run by regrtest.py on Linux pass.Committed asr72909 (trunk),r72910 (py3k).

Thanks!> Committed asr72909 (trunk),r72910 (py3k).> > ----------> resolution: accepted -> fixed> status: open -> closed