From 947a3e0a623589a396362c6f3b7181da4b8175d4 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 6 Jan 2018 21:06:56 +0200 Subject: [PATCH 1/8] bpo-31993: Do not create frames for large bytes and str objects when serialize into memory buffer with C pickle implementations. This optimization already is performed when serialize into memory with Python pickle implementations or into a file with both implementations. --- Modules/_pickle.c | 63 ++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 5cb1fba6cc2918..a40422fae264fa 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2147,11 +2147,9 @@ save_float(PicklerObject *self, PyObject *obj) static int _Pickler_write_large_bytes( PicklerObject *self, const char *header, Py_ssize_t header_size, - PyObject *payload) + const char *data, Py_ssize_t data_size, PyObject *payload) { assert(self->output_buffer != NULL); - assert(self->write != NULL); - PyObject *result; /* Commit the previous frame. */ if (_Pickler_CommitFrame(self)) { @@ -2163,22 +2161,39 @@ _Pickler_write_large_bytes( if (_Pickler_Write(self, header, header_size) < 0) { return -1; } - /* Dump the output buffer to the file. */ - if (_Pickler_FlushToFile(self) < 0) { - return -1; - } - /* Stream write the payload into the file without going through the - output buffer. */ - result = PyObject_CallFunctionObjArgs(self->write, payload, NULL); - if (result == NULL) { - return -1; + if (self->write == NULL) { + if (_Pickler_Write(self, data, data_size) < 0) { + return -1; + } } - Py_DECREF(result); + else { + PyObject *result, *mem = NULL; + /* Dump the output buffer to the file. */ + if (_Pickler_FlushToFile(self) < 0) { + return -1; + } - /* Reinitialize the buffer for subsequent calls to _Pickler_Write. */ - if (_Pickler_ClearBuffer(self) < 0) { - return -1; + /* Stream write the payload into the file without going through the + output buffer. */ + if (payload == NULL) { + payload = mem = PyMemoryView_FromMemory((char *) data, data_size, + PyBUF_READ); + if (payload == NULL) { + return -1; + } + } + result = PyObject_CallFunctionObjArgs(self->write, payload, NULL); + Py_XDECREF(mem); + if (result == NULL) { + return -1; + } + Py_DECREF(result); + + /* Reinitialize the buffer for subsequent calls to _Pickler_Write. */ + if (_Pickler_ClearBuffer(self) < 0) { + return -1; + } } /* Re-enable framing for subsequent calls to _Pickler_Write. */ @@ -2265,7 +2280,7 @@ save_bytes(PicklerObject *self, PyObject *obj) return -1; /* string too large */ } - if (size < FRAME_SIZE_TARGET || self->write == NULL) { + if (size < FRAME_SIZE_TARGET) { if (_Pickler_Write(self, header, len) < 0) { return -1; } @@ -2276,7 +2291,10 @@ save_bytes(PicklerObject *self, PyObject *obj) else { /* Bypass the in-memory buffer to directly stream large data into the underlying file object. */ - if (_Pickler_write_large_bytes(self, header, len, obj) < 0) { + if (_Pickler_write_large_bytes(self, header, len, + PyBytes_AS_STRING(obj), size, + obj) < 0) + { return -1; } } @@ -2364,7 +2382,6 @@ write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) { char header[9]; Py_ssize_t len; - PyObject *mem; assert(size >= 0); if (size <= 0xff && self->proto >= 4) { @@ -2402,15 +2419,9 @@ write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) else { /* Bypass the in-memory buffer to directly stream large data into the underlying file object. */ - mem = PyMemoryView_FromMemory((char *) data, size, PyBUF_READ); - if (mem == NULL) { - return -1; - } - if (_Pickler_write_large_bytes(self, header, len, mem) < 0) { - Py_DECREF(mem); + if (_Pickler_write_large_bytes(self, header, len, data, size, NULL) < 0) { return -1; } - Py_DECREF(mem); } return 0; } From a4394f0f1241d56dce880171497be300e9b47f8a Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 6 Jan 2018 21:21:49 +0200 Subject: [PATCH 2/8] Enable this feature for long strings. --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index a40422fae264fa..387b778d7b9abe 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2408,7 +2408,7 @@ write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) return -1; } - if (size < FRAME_SIZE_TARGET || self->write == NULL) { + if (size < FRAME_SIZE_TARGET) { if (_Pickler_Write(self, header, len) < 0) { return -1; } From ca44d9f6e2e21725fe186fb006b6d668c529f856 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sat, 6 Jan 2018 21:52:32 +0200 Subject: [PATCH 3/8] Refactoring. --- Modules/_pickle.c | 83 +++++++++++++++++++---------------------------- 1 file changed, 33 insertions(+), 50 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 387b778d7b9abe..6e4029b6e5568c 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2142,32 +2142,33 @@ save_float(PicklerObject *self, PyObject *obj) return 0; } -/* No-copy code-path to write large contiguous data directly into the - underlying file object, bypassing the output_buffer of the Pickler. */ +/* Write large contiguous data directly into the underlying file object, + bypassing the output_buffer of the Pickler. */ static int -_Pickler_write_large_bytes( - PicklerObject *self, const char *header, Py_ssize_t header_size, - const char *data, Py_ssize_t data_size, PyObject *payload) +_Pickler_write_bytes(PicklerObject *self, + const char *header, Py_ssize_t header_size, + const char *data, Py_ssize_t data_size, + PyObject *payload) { - assert(self->output_buffer != NULL); + int bypass_framing = (self->framing && data_size >= FRAME_SIZE_TARGET); - /* Commit the previous frame. */ - if (_Pickler_CommitFrame(self)) { - return -1; + if (bypass_framing) { + assert(self->output_buffer != NULL); + /* Commit the previous frame. */ + if (_Pickler_CommitFrame(self)) { + return -1; + } + /* Disable frameing temporarily */ + self->framing = 0; } - /* Disable frameing temporarily */ - self->framing = 0; if (_Pickler_Write(self, header, header_size) < 0) { return -1; } - if (self->write == NULL) { - if (_Pickler_Write(self, data, data_size) < 0) { - return -1; - } - } - else { + if (bypass_framing && self->write != NULL) { + /* Bypass the in-memory buffer to directly stream large data + into the underlying file object. */ PyObject *result, *mem = NULL; /* Dump the output buffer to the file. */ if (_Pickler_FlushToFile(self) < 0) { @@ -2195,9 +2196,16 @@ _Pickler_write_large_bytes( return -1; } } + else { + if (_Pickler_Write(self, data, data_size) < 0) { + return -1; + } + } - /* Re-enable framing for subsequent calls to _Pickler_Write. */ - self->framing = 1; + if (bypass_framing) { + /* Re-enable framing for subsequent calls to _Pickler_Write. */ + self->framing = 1; + } return 0; } @@ -2280,23 +2288,10 @@ save_bytes(PicklerObject *self, PyObject *obj) return -1; /* string too large */ } - if (size < FRAME_SIZE_TARGET) { - if (_Pickler_Write(self, header, len) < 0) { - return -1; - } - if (_Pickler_Write(self, PyBytes_AS_STRING(obj), size) < 0) { - return -1; - } - } - else { - /* Bypass the in-memory buffer to directly stream large data - into the underlying file object. */ - if (_Pickler_write_large_bytes(self, header, len, - PyBytes_AS_STRING(obj), size, - obj) < 0) - { - return -1; - } + if (_Pickler_write_bytes(self, header, len, + PyBytes_AS_STRING(obj), size, obj) < 0) + { + return -1; } if (memo_put(self, obj) < 0) @@ -2408,20 +2403,8 @@ write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) return -1; } - if (size < FRAME_SIZE_TARGET) { - if (_Pickler_Write(self, header, len) < 0) { - return -1; - } - if (_Pickler_Write(self, data, size) < 0) { - return -1; - } - } - else { - /* Bypass the in-memory buffer to directly stream large data - into the underlying file object. */ - if (_Pickler_write_large_bytes(self, header, len, data, size, NULL) < 0) { - return -1; - } + if (_Pickler_write_bytes(self, header, len, data, size, NULL) < 0) { + return -1; } return 0; } From 2a489d98f9c5b8ba7a7a01234dc22d6fbb4c32ac Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Sun, 7 Jan 2018 18:37:57 +0200 Subject: [PATCH 4/8] Fix several bugs and update a comment. --- Modules/_pickle.c | 79 +++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 41 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 6e4029b6e5568c..c1928750916e0b 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2142,17 +2142,23 @@ save_float(PicklerObject *self, PyObject *obj) return 0; } -/* Write large contiguous data directly into the underlying file object, - bypassing the output_buffer of the Pickler. */ +/* Perform direct write of the header and payload of the binary object. + + The large contiguous data is written directly into the underlying file + object, bypassing the output_buffer of the Pickler. We intentionally + do not insert a protocol 4 frame opcode to make it possible to optimize + file.read calls in the loader. + */ static int _Pickler_write_bytes(PicklerObject *self, const char *header, Py_ssize_t header_size, const char *data, Py_ssize_t data_size, PyObject *payload) { - int bypass_framing = (self->framing && data_size >= FRAME_SIZE_TARGET); + int bypass_buffer = (data_size >= FRAME_SIZE_TARGET); + int framing = self->framing; - if (bypass_framing) { + if (bypass_buffer) { assert(self->output_buffer != NULL); /* Commit the previous frame. */ if (_Pickler_CommitFrame(self)) { @@ -2166,7 +2172,7 @@ _Pickler_write_bytes(PicklerObject *self, return -1; } - if (bypass_framing && self->write != NULL) { + if (bypass_buffer && self->write != NULL) { /* Bypass the in-memory buffer to directly stream large data into the underlying file object. */ PyObject *result, *mem = NULL; @@ -2178,8 +2184,7 @@ _Pickler_write_bytes(PicklerObject *self, /* Stream write the payload into the file without going through the output buffer. */ if (payload == NULL) { - payload = mem = PyMemoryView_FromMemory((char *) data, data_size, - PyBUF_READ); + payload = mem = PyBytes_FromStringAndSize(data, data_size); if (payload == NULL) { return -1; } @@ -2202,10 +2207,8 @@ _Pickler_write_bytes(PicklerObject *self, } } - if (bypass_framing) { - /* Re-enable framing for subsequent calls to _Pickler_Write. */ - self->framing = 1; - } + /* Re-enable framing for subsequent calls to _Pickler_Write. */ + self->framing = framing; return 0; } @@ -2373,10 +2376,29 @@ raw_unicode_escape(PyObject *obj) } static int -write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) +write_unicode_binary(PicklerObject *self, PyObject *obj) { char header[9]; Py_ssize_t len; + PyObject *encoded = NULL; + Py_ssize_t size; + const char *data; + + if (PyUnicode_READY(obj)) + return -1; + + data = PyUnicode_AsUTF8AndSize(obj, &size); + if (data == NULL) { + /* Issue #8383: for strings with lone surrogates, fallback on the + "surrogatepass" error handler. */ + PyErr_Clear(); + encoded = PyUnicode_AsEncodedString(obj, "utf-8", "surrogatepass"); + if (encoded == NULL) + return -1; + + data = PyBytes_AS_STRING(encoded); + size = PyBytes_GET_SIZE(encoded); + } assert(size >= 0); if (size <= 0xff && self->proto >= 4) { @@ -2400,43 +2422,18 @@ write_utf8(PicklerObject *self, const char *data, Py_ssize_t size) else { PyErr_SetString(PyExc_OverflowError, "cannot serialize a string larger than 4GiB"); + Py_XDECREF(encoded); return -1; } - if (_Pickler_write_bytes(self, header, len, data, size, NULL) < 0) { + if (_Pickler_write_bytes(self, header, len, data, size, encoded) < 0) { + Py_XDECREF(encoded); return -1; } + Py_XDECREF(encoded); return 0; } -static int -write_unicode_binary(PicklerObject *self, PyObject *obj) -{ - PyObject *encoded = NULL; - Py_ssize_t size; - const char *data; - int r; - - if (PyUnicode_READY(obj)) - return -1; - - data = PyUnicode_AsUTF8AndSize(obj, &size); - if (data != NULL) - return write_utf8(self, data, size); - - /* Issue #8383: for strings with lone surrogates, fallback on the - "surrogatepass" error handler. */ - PyErr_Clear(); - encoded = PyUnicode_AsEncodedString(obj, "utf-8", "surrogatepass"); - if (encoded == NULL) - return -1; - - r = write_utf8(self, PyBytes_AS_STRING(encoded), - PyBytes_GET_SIZE(encoded)); - Py_DECREF(encoded); - return r; -} - static int save_unicode(PicklerObject *self, PyObject *obj) { From b89cea163866f0505d08b5daa9dbd6e0c4628c2b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 8 Jan 2018 17:19:31 +0200 Subject: [PATCH 5/8] Fix a typo and defer fixing the memoryview issue to other PR. --- Modules/_pickle.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index c1928750916e0b..495c223aa35811 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2184,7 +2184,8 @@ _Pickler_write_bytes(PicklerObject *self, /* Stream write the payload into the file without going through the output buffer. */ if (payload == NULL) { - payload = mem = PyBytes_FromStringAndSize(data, data_size); + payload = mem = PyMemoryView_FromMemory((char *) data, data_size, + PyBUF_READ); if (payload == NULL) { return -1; } From d0a9a18944022520a3515d6fe05f77bd1ecc8b3a Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 8 Jan 2018 17:52:32 +0200 Subject: [PATCH 6/8] Make test_framing_large_objects covering this case. --- Lib/test/pickletester.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 5d983eb617fe3f..4ceb5dc38a97ee 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -2097,20 +2097,18 @@ def test_framing_large_objects(self): N = 1024 * 1024 obj = [b'x' * N, b'y' * N, 'z' * N] for proto in range(4, pickle.HIGHEST_PROTOCOL + 1): - for fast in [True, False]: + for fast in [True, False, None]: with self.subTest(proto=proto, fast=fast): - if hasattr(self, 'pickler'): + if fast is None: + pickled = self.dumps(obj, proto) + else: + if not hasattr(self, 'pickler'): + continue buf = io.BytesIO() pickler = self.pickler(buf, protocol=proto) pickler.fast = fast pickler.dump(obj) pickled = buf.getvalue() - elif fast: - continue - else: - # Fallback to self.dumps when fast=False and - # self.pickler is not available. - pickled = self.dumps(obj, proto) unpickled = self.loads(pickled) # More informative error message in case of failure. self.assertEqual([len(x) for x in obj], From 5551b6f5515b0a331dae58d7676bda489e701d09 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 8 Jan 2018 18:13:41 +0200 Subject: [PATCH 7/8] Fix a typo again. --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 495c223aa35811..5b3de4d96dda02 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -2164,7 +2164,7 @@ _Pickler_write_bytes(PicklerObject *self, if (_Pickler_CommitFrame(self)) { return -1; } - /* Disable frameing temporarily */ + /* Disable framing temporarily */ self->framing = 0; } From 4145cfcebdc63d6fe004af6148f693a777a557d5 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Tue, 9 Jan 2018 14:17:16 +0200 Subject: [PATCH 8/8] Update tests. --- Lib/test/pickletester.py | 7 +++++-- Lib/test/test_pickle.py | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 4ceb5dc38a97ee..f4e3f81249c86e 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -2097,11 +2097,14 @@ def test_framing_large_objects(self): N = 1024 * 1024 obj = [b'x' * N, b'y' * N, 'z' * N] for proto in range(4, pickle.HIGHEST_PROTOCOL + 1): - for fast in [True, False, None]: + for fast in [False, True]: with self.subTest(proto=proto, fast=fast): - if fast is None: + if not fast: + # fast=False by default. + # This covers in-memory pickling with pickle.dumps(). pickled = self.dumps(obj, proto) else: + # Pickler is required when fast=True. if not hasattr(self, 'pickler'): continue buf = io.BytesIO() diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py index 895ed48df1d8d2..0051a0118a3f21 100644 --- a/Lib/test/test_pickle.py +++ b/Lib/test/test_pickle.py @@ -71,8 +71,6 @@ def loads(self, buf, **kwds): class InMemoryPickleTests(AbstractPickleTests, AbstractUnpickleTests, BigmemPickleTests): - pickler = pickle._Pickler - unpickler = pickle._Unpickler bad_stack_errors = (pickle.UnpicklingError, IndexError) truncated_errors = (pickle.UnpicklingError, EOFError, AttributeError, ValueError, @@ -84,6 +82,8 @@ def dumps(self, arg, protocol=None): def loads(self, buf, **kwds): return pickle.loads(buf, **kwds) + test_framed_write_sizes_with_delayed_writer = None + class PersistentPicklerUnpicklerMixin(object):