ENH: derive from C-pickler for fast serialization

[pierreglaser]

Summary:

This PR proposes a new Cloudpickler class, that inherits from the C
_pickle.Pickler instead of the python pickle._Pickler, allowing 10x+
speedups for the serialization of large builtin objects such as dicts, lists..

Disclaimer: a new start

Moving from the python to the c Pickler requires a fair amount of changes.
For this reason, instead of simply adapting the current code to respect the new
constraints, I started back from scratch. This allows a new, clean API and
structure, that will be hopefully easier to understand for everyone.

I made a lot of comments, (sometimes overly verbose), to ease the review
process of this PR. Eventually, I hope the information they contain can be
transfered to a proper project documentation.

Implementation:

Changes to python

As opposed to the python pickler, The CPickler does not expose the save_*
family of functions, as well as low level isntructions such as write. These
methods can can neither be patched, or called, and the only customization
option we had initially was the dispatch table, that is called for all types
BUT a few special cases, including classes and functions, the two principal
use-cases of cloudpickle.

As this makes it simply impossible to modify pickling behavior for such types,
we patched the C pickler for it to allow a user defined reduction callback for
functions and classes. This idea was suggested by @pitrou.

The direct consequence is that functions and classes now have to follow the
save_reduce-load_build pickling/depickling process. Unfortunaltely,
this API is not well suited for custom builtin-type saving: in particular,
the state setting part of load_build (function that reconstructs an object from
a reduce value) assumes all attributes of an object are writeable, which is not
the case for C types (especially function.__globals__ and
function.__closure__)

For this reason, we also changed the API of save_reduce, allowing to add a
custom state_setter, that will be called at unpickling time.

You can view the totals changes in this diff

Individual PRs to CPython:

• bpo-35900: Enable custom reduction callback registration in _pickle python/cpython#12499 (reducer_override)
• bpo-35900: Add a state_setter arg to save_reduce python/cpython#12588 (state_setter in save_reduce)

Changes to cloudpickle

Functions and classes are the two main types affected by this PR. The main
challenge was to make the saving process fit into the save_reduce API.

Outside of these types, the actuall reduction process remains intact.

However, now that any customization must return a tuple, I decided to adopt a
new naming, hopefully clearer naming style for functions. You will see by
yourselves.

How to build this version locally

Until the final release of Python 3.8, you need to build python from upstream's master branch

git clone git@github.com:python/cpython.git
cd cpython
./configure
make

To be able to use external modules you need a virtual environment, using for
example the venv module:

./python -m venv /path/to/local/virtualenv

Clone and install cloudpickle and its dependencies

cd /path/to/cloudpickle
git clone git@github.com:cloudpipe/cloudpickle.git
git fetch origin pull/ID/head:fast-cloudpickle
/path/to/local/virtualenv/bin/python -mpip install -rdev-requirements.txt
/path/to/local/virtualenv/bin/python -mpip install .

Finally, rum the tests:

/path/to/local/virtualenv/bin/python -mpytest tests/

Bechmarks:

• Benchmarks of a "concrete", end-to-end use-case using loky can be found
  here. To run the benchmarks, you also need the master version of loky.

[dimosped]

Thank you for putting this PR together.
Do you have an expected date this will merge on master?

Also any plans for making this change backwards compatible < 3.8 or even 2.7 ?

[pierreglaser]

Do you have an expected date this will merge on master?

It will all depend on whether cpython core developers agree on the patches we proposed. I would say (rough estimate) between zero and two months.

Also any plans for making this change backwards compatible < 3.8 or even 2.7 ?

This is still an open question: doing this would mean including the new _pickle.c code to cloudpickle and maintaining it.

[ogrisel]

This is still an open question: doing this would mean including the new _pickle.c code to cloudpickle and maintaining it.

Another option (for Python 3.6+ only) would be to include the change in the pickle5 backport and make it a soft dependency for cloudpickle.

[ogrisel]

I think all the new helpers functions and reducers should be made private (with a leading _ in their name so as to make it explicit that this is not part of the public API and can change without a deprecation notice).

Furthermore I find this PR a bit hard to review because some the helpers are duplicates from existing helpers in cloudpickle.cloudpickle and it's hard to know if they are exactly the same or not. Hence I suggest the following:

[pitrou]

@pierreglaser I'm sorry about my bad memory, but did you post a PR and/or a Python issue about your changes? I don't remember.

If a change is needed in pickle protocol 5, you'd better tell me also (and/or post a thread on python-dev to gather feedback).

[pitrou]

Another option (for Python 3.6+ only) would be to include the change in the pickle5 backport and make it a soft dependency for cloudpickle.

That would be theoretically ok with me. Whether I concretely agree or not depends on how harder it makes to maintain the backport (right now it works with simple diff scripts and bit a manual work whenever there's a conflict).

[pierreglaser]

did you post a PR and/or a Python issue about your changes?

@pitrou there is an issue in the bug tracker, with patches. I was waiting for feedback before submitting a PR.

If a change is needed in pickle protocol 5, you'd better tell me also

There is no change to the pickle protocol (no new opcode) itself, however I extended the save_reduce API, by adding a state_setter keyword argument.

[pitrou]

Nowadays PRs are easier to read than raw patches, so I'd rather have that.

[ogrisel]

It seems that this PR needs to be updated to take the merge of #254 into account.

[ogrisel]

A small batch of comments:

[ogrisel]

Here is a first review. Overall this looks good to me.

Have you tried to run some of the downstream test suites? I guess we have to do that manually because of the need to run against a patched python master.

[pierreglaser]

The more I think about it, the less I am convinced about the need for globals_ref to be a CloudPickler attribute. Having globals_ref as a cloudpickle global variable will not impact the CloudPickler memoization process, and allows us to get rid of this weird signature for functions-related reducers. @ogrisel WDYT?

[ogrisel]

We don't want to accumulate objects in a global datastructure. This can be considered a memory leak (although there would be only empty dicts there if I am correct).

I much prefer to avoid global variables if there is a natural way not have any (as in the current code).

[pierreglaser]

@ogrisel will cloudpickle.cloudpickle and cloudpickle.cloudpickle_fast be considered private after this PR is merged? More precisely, should cloudpickle.cloudpickle still be expected to work on Python versions >= 3.8?

(the CI breaks because cloudpickle.cloudpickle instanciantes a global variable - _cell_set_template_code - using the types.CodeType constructor, whose signature changed in 3.8 to allow for positional-only parameters)

[ogrisel]

More precisely, should cloudpickle.cloudpickle still be expected to work on Python versions >= 3.8?

I think it would be a good idea to ensure forward compatibility for the pure Python Pickler implementation if it's not to complicated to get.

[ogrisel]

I merged #246 which will impact this PR significantly unfortunately.

[ogrisel]

@pierreglaser would it be easier for your to redo this PR only once #262 is merged? In which case I would put the priority on reviewing it.

[ogrisel]

This comment #253 (comment) does not seem to be addressed.

[pierreglaser]

There was a race condition with your comment and my last commit :) I'll cherry-pick in a separate PR.

[ogrisel]

Haha, as you wish. Please add an entry to the changelog so that we can merge this PR :)

[pierreglaser]

I changed my mind I'll leave it as is, as this is part of the refactoring of this PR to change this helper. I'll add an entry to the changelog and make a last pass.

[pierreglaser]

@ogrisel this LGTM as well!

[ogrisel]

Merged! Thank you very much @pierreglaser!