bpo-31993: Do not create frames for large bytes and str objects#5114
Conversation
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.
| /* 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, |
There was a problem hiding this comment.
What does this change? If self->write == NULL, It calls _Pickler_Write(self, header, len) < 0) followed by _Pickler_Write(self, PyBytes_AS_STRING(obj), size) exactly as above.
There was a problem hiding this comment.
But it first commits the current frame and temporary disables framing.
|
|
||
| /* 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, |
There was a problem hiding this comment.
Can you rephrase the comments now that this is called also when there is no underlying file object?
See comments in pickle.py for inspiration.
|
Using |
ogrisel
left a comment
ogrisel
left a comment
There was a problem hiding this comment.
The refactoring looks good to me. Maybe we should add a test to check that the number of frames is not too large in a pickled list of large bytes instances. For instance using count_opcode(pickle.FRAME, pickled) in pickletester.py.
Please also find a comment below in the memory copy induced by PyBytes_FromStringAndSize when dumping large unicode objects.
| if (_Pickler_CommitFrame(self)) { | ||
| return -1; | ||
| } | ||
| /* Disable frameing temporarily */ |
| /* Stream write the payload into the file without going through the | ||
| output buffer. */ | ||
| if (payload == NULL) { | ||
| payload = mem = PyBytes_FromStringAndSize(data, data_size); |
There was a problem hiding this comment.
This causes an extra memory copy for large str (unicode) objects (compared to the unsafe previous code that relied on PyMemoryView_FromMemory).
This can be measured with:
https://gist.github.com/ogrisel/0e7b3282c84ae4a581f3b9ec1d84b45a
But I am not sure how to write a safe version of the pickling code that would not cause this extra copy. If it's not possible, the entry for bpo-31993 in the changelog should probably be updated.
There was a problem hiding this comment.
At least I would like this to go to a separate PR. Please let's not mix concerns.
|
Thank you for your review. |
| with self.subTest(proto=proto, fast=fast): | ||
| if hasattr(self, 'pickler'): | ||
| if fast is None: | ||
| pickled = self.dumps(obj, proto) |
There was a problem hiding this comment.
What is the point of the None test? Can you please add a comment?
5 participants
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.
https://bugs.python.org/issue31993