bpo-31993: Do not use memoryview when pickle large strings. #5154
Conversation
PyMemoryView_FromMemory() created a memoryview referring to the internal data of the string. When the string is destroyed the memoryview become referring to a freed memory.
Lib/test/pickletester.py
Outdated
| @@ -2179,57 +2179,60 @@ def write(self, chunk): | |||
| def concatenate_chunks(self): | |||
| # Some chunks can be memoryview instances, we need to convert | |||
| # them to bytes to be able to call join | |||
There was a problem hiding this comment.
This comment should now be removed.
Lib/test/pickletester.py
Outdated
|
|
||
| # Actually read the binary content of the chunks after the end | ||
| # of the call to dump: ant memoryview passed to write should not | ||
| # of the call to dump: and memoryview passed to write should not |
There was a problem hiding this comment.
I think this should read "any memoryview" instead.
Lib/test/pickletester.py
Outdated
| self.assertGreaterEqual(9, chunk_size) | ||
| self.assertLess(chunk_size, 2 * self.FRAME_SIZE_TARGET, | ||
| chunk_sizes) | ||
| # There shouldn't bee too much small chunks. |
There was a problem hiding this comment.
Maybe add a comment: the protocol header, the frame headers and the large string headers are written in small chunks.
Modules/_pickle.c
Outdated
| @@ -2184,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); | |||
There was a problem hiding this comment.
This forces a memory copy. Wouldn't it be possible to create a read-only memoryview that keeps a reference to the original ascii str object to avoid the large memory overhead?
There was a problem hiding this comment.
This is not easy. Currently there is no API for creating a memoryview to the area of memory with linked Python object. Designing such API is a separate issue. There should be other issues on the tracker that needs this API. For example bytes IO seems suffers from this issue.
Here I just try to fix a regression introduced by #4353.
Modules/_pickle.c
Outdated
| @@ -2184,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); | |||
There was a problem hiding this comment.
This is not easy. Currently there is no API for creating a memoryview to the area of memory with linked Python object. Designing such API is a separate issue. There should be other issues on the tracker that needs this API. For example bytes IO seems suffers from this issue.
Here I just try to fix a regression introduced by #4353.
|
This looks fine to me. Does the previous NEWS file need updating? |
Misc/NEWS.d/3.7.0a4.rst
Outdated
| The picklers no longer allocate temporary memory when dumping large | ||
| ``bytes`` and ``str`` objects into a file object. Instead the data is | ||
| directly streamed into the underlying file object. | ||
| The pickler now uses less memory when serialize large bytes and str |
PyMemoryView_FromMemory()created a memoryview referring to the internal data of the string. When the string is destroyed the memoryview become referring to a freed memory.https://bugs.python.org/issue31993