bpo-31993: do not allocate large temporary buffers in pickle dump by ogrisel · Pull Request #4353 · python/cpython

ogrisel · 2017-11-09T18:14:38Z

For all protocols: avoid concatenating large bytes and str with their opcode
and size header but instead issue an individual call to self.write(data).

For protocol 4: if the size of the opcode + size header + data is larger than
the target frame size, commit the current frame and bypass io.BytesIO to write
the next frame directly to the underlying file object.

https://bugs.python.org/issue31993

Edit: this now includes changes both the Python and C implementations of the picklers.

the-knights-who-say-ni · 2017-11-09T18:14:41Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

pitrou · 2017-11-09T18:19:37Z

        else:
            return self.file_write(data)

+    def write_chunks(self, *chunks):


Nit: I would call this write_many.

pitrou · 2017-11-09T18:23:39Z

+        if self.current_frame and total_size >= self._FRAME_SIZE_TARGET:
+            # Terminate the current frame to write the next frame directly into
+            # the underlying file to skip the unnecessary memory allocations
+            # of a large temporary buffers.


Typo: "a large temporary buffers".

pitrou · 2017-11-09T18:23:53Z

+        else:
+            write = self.write
+
+        for chunk in chunks:


Add a comment pointing out that you avoid concatenation?

serhiy-storchaka · 2017-11-09T18:28:40Z

I don't think this is needed for short bytes and strings.

serhiy-storchaka · 2017-11-09T18:32:14Z

It may be better to concatenate the opcode and the size.

ogrisel · 2017-11-09T18:41:34Z

Thanks for the quick review.

I think I addressed your comments. I am not sure how to write the entry in NEWS.d. I think this PR is useful as it is and we should not wait for the C version. Shall I add an entry for the Python _Pickler alone in the NEWS file for 3.7.0a2?

I would appreciate it if someone else could do the C version.

pitrou · 2017-11-09T22:28:27Z

Yes, you should add a NEWS entry. You can (and should) use the "blurb" utility for that.

I think it's fine to leave the C version for later or to someone else if you're not comfortable with that.

ogrisel · 2017-11-09T23:08:00Z

Done. Feel free to edit it directly if you want to change anything.

serhiy-storchaka · 2017-11-10T15:01:11Z

I'm not sure about 5d46315. opcode and size_header are concatenated in any case. If there is any benefit from this change, how can it be explained?

serhiy-storchaka · 2017-11-10T15:07:54Z

Ah, you already have reverted this change. The final variant looks pretty simple.

ogrisel · 2017-11-10T15:59:52Z

Yes sorry I had pushed a bad intermediate version and then rebased on top of master to make sure I was benchmarking against the same master as yours. I think the intermediate commits are useless, do you want me to squash them all into a single commit?

serhiy-storchaka · 2017-11-10T16:03:12Z

Don't squash them. It is more convenient to make a review if intermediate commits are not squashed. All them will be squashed when be merged in master.

ogrisel · 2017-11-10T16:16:20Z

Alright.

ogrisel · 2017-11-11T15:41:02Z

@serhiy-storchaka approved this PR but I subsequently included an implementation for the C version that has not been reviewed yet. The "awaiting-merge" label should be removed and replaced by "awaiting-review".

pitrou · 2017-11-11T18:29:39Z

Why don't you simply pass the payload as a const char * instead?

Oh, I see, it's because you pass it to self->write.

ogrisel · 2017-11-12T11:28:51Z

I assumed that this is not part of the public API so it's fine to change the prototype of this function without caring about backward compat. Let me know if this is not the case.

All static functions are private.

ogrisel · 2017-11-12T11:32:14Z

Do you want me to convert this script into a new non-regression test that ensures that future changes to the pickle module will not break the no-copy semantics dump for large bytes & utf-8 backed str?

serhiy-storchaka

Excellent! I didn't expect that you will go so far! And the resulting code is pretty simple.

Added few style comments and one question.

serhiy-storchaka · 2017-11-12T11:48:22Z

Some of your comments ends with a period, but others do not. Would be better to be uniform.

serhiy-storchaka · 2017-11-12T11:54:27Z

Is it worth to create a new frame containing a single bytes/string object? This will increase the size of the file, but will not decrease the number of file reading operations (opcode + frame/bytes/string size + payload).

I think the presence of a frame is required by protocol 4. The overhead is likely to be small because this code path is only there for large enough bytes instances.

PEP 3154 is not very explicit whether framing is required or optional. The existing implementations (Python and C) would never dump a frameless opcode as far as I understand so it might be safer to follow that behavior in the new nocopy code path but I can change that if you think this is being too conservative.

I remember that some people voiced concerns about the frame header overhead (however small it is), so I worded the PEP so that it isn't made mandatory (and the unpickler should be able to handle both with and without frames). But I do think the standard pickler implementations should always emit frames in protocol 4.

Frames are optional.

If don't create a frame for a separate bytes object, this could decrease memory usage on unpickling. You could directly use the result of the read() method without creating a buffer.

Indeed I observed at least one useless memory copy at load time for large objects. I have not investigated yet, however, it's there both with the frameless protocol 3 and with protocol 4 with framing enabled. Anyway, it would be best to choose a design that allows for no-copy semantics of large objects both at dump and load times.

serhiy-storchaka · 2017-11-12T11:55:29Z

There are no other picklers except the C-based and Python-based picklers. 😸

serhiy-storchaka · 2017-11-12T11:57:10Z

All static functions are private.

serhiy-storchaka · 2017-11-12T12:00:24Z

New PEP 7 rule requires braces even around the if body containing a simple return statement. The code in this module is older than this rule, but the new code should obey it.

serhiy-storchaka · 2017-11-12T12:01:25Z

The same doubt as about the Python code. Is creating a new frame needed here?

pitrou · 2017-12-13T17:18:15Z

It would be more interesting to see a benchmark with larger objects (for example just a bit smaller than 64kiB?).

ogrisel · 2017-12-13T21:42:57Z

I ran a benchmark with 63 MB of 63kB random bytes objects and there is no performance regression when using getvalue in this PR vs master:

master:

# protocol 4, output: output.pkl
median time: 0.23s
# protocol 4, output: /dev/null
median time: 0.0262s
last pickle size: 63.009MB

this PR with getvalue instead of getbuffer

# protocol 4, output: output.pkl
median time: 0.23s
# protocol 4, output: /dev/null
median time: 0.0256s
last pickle size: 63.009MB

with getbuffer it's the same speed as master. So maybe getvalue is slightly faster but the difference is probably not significant.

ogrisel · 2017-12-14T14:15:18Z

@pitrou do you want me to push the variant with the call to getvalue()?

pitrou · 2017-12-14T14:18:03Z

No, let's keep getbuffer. I don't think there's any reason to use getvalue. The two alternatives are:

use getbuffer and release the buffer immediately (so that a writer wanting to keep the buffer in memory has to make a copy first)
use getbuffer and not release it

Imposing a copy on everyone is pointless.

ogrisel · 2017-12-18T09:40:19Z

I am in favor of:

use getbuffer and not release it

that is: what is currently implemented in this PR. This way the Python-based pickler behaves closer to the C implementation that allows delayed access to the bytes of the frame contents without having to force a copy.

pitrou

I think this is mostly good to go now. Just two small comments.

pitrou · 2017-12-21T18:07:47Z

+static int
+_Pickler_write_large_bytes(
+    PicklerObject *self, const char *header, Py_ssize_t header_size,
+    PyObject *payload, Py_ssize_t payload_size)


payload_size doesn't seem used below.

pitrou · 2017-12-21T18:08:21Z

+        if(_Pickler_CommitFrame(self)) {
+            return -1;
+        }
+        if (self->write != NULL) {


Would be nice to add a comment here.

bedevere-bot · 2017-12-21T18:09:03Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ogrisel · 2018-01-04T09:00:19Z

@pitrou @serhiy-storchaka I have made the requested changes; please review again.

bedevere-bot · 2018-01-04T09:00:22Z

Thanks for making the requested changes!

@serhiy-storchaka, @pitrou: please review the changes made to this pull request.

pitrou · 2018-01-04T18:37:01Z

Thanks for your work @ogrisel ! Unless @serhiy-storchaka chimes in soon I'm gonna merge this pull request.

serhiy-storchaka · 2018-01-04T18:43:04Z

Let me to look on this again.

serhiy-storchaka

LGTM! I have found just few typos.

I don't have opinion about getbuffer/getvalue. Let defer it until we encounter concrete issue.

serhiy-storchaka · 2018-01-05T11:48:26Z

-                f.truncate()
+                data = f.getbuffer()
+                write = self.file_write
+                # Issue a single call to the write nethod of the underlying


s/nethod/method/

serhiy-storchaka · 2018-01-05T12:14:47Z

+         * to limit memory usage when dumping large complex objects to
+         * a file.
+         *
+         * self-write is NULL when called via dumps.


self->write

ogrisel · 2018-01-05T12:52:04Z

Thanks for the review @serhiy-storchaka, I fixed the remaining typos.

the-knights-who-say-ni added the CLA not signed label Nov 9, 2017

bedevere-bot added the awaiting review label Nov 9, 2017

pitrou reviewed Nov 9, 2017

View reviewed changes

ogrisel force-pushed the issue-31993-pypickle-dump-mem-optim branch from 2472889 to 42da6b6 Compare November 9, 2017 18:25

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 9, 2017

serhiy-storchaka reviewed Nov 9, 2017

View reviewed changes

ogrisel force-pushed the issue-31993-pypickle-dump-mem-optim branch 2 times, most recently from abde3f3 to d0ee85f Compare November 10, 2017 14:36

serhiy-storchaka approved these changes Nov 10, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Nov 10, 2017

pitrou reviewed Nov 11, 2017

View reviewed changes

serhiy-storchaka self-requested a review November 11, 2017 20:39

serhiy-storchaka added the type-feature A feature request or enhancement label Nov 11, 2017

ogrisel commented Nov 12, 2017

View reviewed changes

serhiy-storchaka reviewed Nov 12, 2017

View reviewed changes

pitrou requested changes Dec 21, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Dec 21, 2017

ogrisel added 4 commits January 4, 2018 09:32

Add comment to explain flush after commit in _Pickler_OpcodeBoundary

a2e87e9

Removed unused argument payload_size

8edaa64

Update the changelog to reflect the new behavior of the Python pickler

609ebfe

Fix phrasing

28f6297

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 4, 2018

pitrou approved these changes Jan 4, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 4, 2018

serhiy-storchaka approved these changes Jan 5, 2018

View reviewed changes

Fixed typos in comments

7c91f06

serhiy-storchaka approved these changes Jan 5, 2018

View reviewed changes

serhiy-storchaka merged commit 3cd7c6e into python:master Jan 6, 2018

bedevere-bot removed the awaiting merge label Jan 6, 2018

ogrisel deleted the issue-31993-pypickle-dump-mem-optim branch January 6, 2018 16:47

serhiy-storchaka mentioned this pull request Jan 11, 2018

bpo-31993: Do not use memoryview when pickle large strings. #5154

Merged

Uh oh!

Conversation

ogrisel commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Nov 9, 2017

Uh oh!

pitrou commented Nov 9, 2017

Uh oh!

ogrisel commented Nov 9, 2017

Uh oh!

serhiy-storchaka commented Nov 10, 2017

Uh oh!

serhiy-storchaka commented Nov 10, 2017

Uh oh!

ogrisel commented Nov 10, 2017

Uh oh!

serhiy-storchaka commented Nov 10, 2017

Uh oh!

ogrisel commented Nov 10, 2017

Uh oh!

ogrisel commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Dec 13, 2017

Uh oh!

ogrisel commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogrisel commented Dec 14, 2017

ogrisel commented Nov 9, 2017 •

edited

Loading

ogrisel commented Nov 11, 2017 •

edited

Loading

ogrisel commented Nov 12, 2017 •

edited

Loading

ogrisel Nov 12, 2017 •

edited

Loading

ogrisel commented Dec 13, 2017 •

edited

Loading