BytesIO copy-on-write

[dw]

BPO	22003
Nosy	@pitrou, @scoder, @benjaminp, @skrah, @hynek, @dw, @serhiy-storchaka
Files	cow.patch: BytesIO patch against hg 4c2f3240ad65 cow2.patch: cow.patch version 2 against hg 4c2f3240ad65 cow3.patch: cow.patch version 3 against hg 4c2f3240ad65 cow4.patch: cow.patch version 4 against hg 2fc379ce5762 cow5.patch: cow.patch version 5 against hg 96ea15ee8525 cow6.patch: cow.patch version 6 against hg 8c1438c15ed0 whatsnew.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-07-29.23:46:30.037>
created_at = <Date 2014-07-17.22:25:37.100>
labels = ['library', 'performance']
title = 'BytesIO copy-on-write'
updated_at = <Date 2015-03-04.21:04:29.471>
user = 'https://github.com/dw'

bugs.python.org fields:

activity = <Date 2015-03-04.21:04:29.471>
actor = 'dw'
assignee = 'none'
closed = True
closed_date = <Date 2014-07-29.23:46:30.037>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2014-07-17.22:25:37.100>
creator = 'dw'
dependencies = []
files = ['35988', '36004', '36005', '36016', '36078', '36137', '38058']
hgrepos = []
issue_num = 22003
keywords = ['patch']
message_count = 37.0
messages = ['223383', '223385', '223386', '223401', '223402', '223522', '223526', '223542', '223581', '223582', '223583', '223586', '223588', '223599', '223600', '223611', '223633', '223681', '223682', '223683', '223692', '223707', '223907', '223962', '224121', '224139', '224164', '224168', '224169', '224238', '224273', '224274', '235596', '235619', '236002', '237207', '237212']
nosy_count = 11.0
nosy_names = ['pitrou', 'scoder', 'benjamin.peterson', 'stutzbach', 'skrah', 'python-dev', 'hynek', 'piotr.dobrogost', 'dw', 'serhiy.storchaka', 'kmike']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue22003'
versions = ['Python 3.5']

[dw]

This is a followup to the thread at https://mail.python.org/pipermail/python-dev/2014-July/135543.html , discussing the existing behaviour of BytesIO copying its source object, and how this regresses compared to cStringIO.StringI.

The goal of posting the patch on list was to try and stimulate discussion around the approach. The patch itself obviously isn't ready for review, and I'm not in a position to dedicate time to it just now (although in a few weeks I'd love to give it full attention!).

Ignoring this quick implementation, are there any general comments around the approach?

My only concern is that it might keep large objects alive in a non-intuitive way in certain circumstances, though I can't think of any obvious ones immediately.

Also interested in comments on the second half of that thread: "a natural extension of this is to do something very similar on the write side: instead of generating a temporary private heap allocation, generate (and freely resize) a private PyBytes object until it is exposed to the user, at which point, _getvalue() returns it, and converts its into an IO_SHARED buffer."

There are quite a few interactions with making that work correctly, in particular:

• How BytesIO would implement the buffers interface without causing the under-construction Bytes to become readonly

• Avoiding redundant copies and resizes -- we can't simply tack 25% slack on the end of the Bytes and then truncate it during getvalue() without likely triggering a copy and move, however with careful measurement of allocator behavior there are various tradeoffs that could be made - e.g. obmalloc won't move a <500 byte allocation if it shrinks by <25%. glibc malloc's rules are a bit more complex though.

Could also add a private _PyBytes_SetSize() API to allow truncation to the final size during getvalue() without informing the allocator. Then we'd simply overallocate by up to 10% or 1-2kb, and write off the loss of the slack space.

Notably, this approach completely differs from the one documented in http://bugs.python.org/issue15381 .. it's not clear to me which is better.

[dw]

Submitted contributor agreement. Please consider the demo patch licensed under the Apache 2 licence.

[pitrou]

Be careful what happens when the original object is mutable:

>>> b = bytearray(b"abc")
>>> bio = io.BytesIO(b)
>>> b[:] = b"defghi"
>>> bio.getvalue()
b'abc'

I don't know what your patch does in this case.

[dw]

Good catch :( There doesn't seem to be way a to ask for an immutable buffer, so perhaps it could just be a little more selective. I think the majority of use cases would still be covered if the sharing behaviour was restricted only to BytesType.

In that case "Py_buffer initialdata" could become a PyObject*, saving a small amount of memory, and allowing reuse of the struct member if BytesIO was also modified to directly write into a private BytesObject

[scoder]

Even if there is no way to explicitly request a RO buffer, the Py_buffer struct that you get back actually tells you if it's read-only or not. Shouldn't that be enough to enable this optimisation?

Whether or not implementors of the buffer protocol set this flag correctly is another question, but if not then they need fixing on their side anyway. (And in the vast majority of cases, the implementor will be either CPython or NumPy.)

Also, generally speaking, I think such an optimisation would be nice, even if it only catches some common cases (and doesn't break the others :). It could still copy data if necessary, but try to avoid it if possible.

[dw]

This version is tidied up enough that I think it could be reviewed.

Changes are:

• Defer `buf' allocation until __init__, rather than __new__ as was previously done. Now upon completion, BytesIO.__new__ returns a valid, closed BytesIO, whereas previously a valid, empty, open BytesIO was returned. Is this interface change permissible?

• Move __init__ guts into a "reinit()", for sharing with __setstate__, which also previously caused an unnecessary copy. Additionally gather up various methods for deallocating buffers into a single "reset()" function, called by reinit(), _dealloc(), and _close()

• Per Stefan's suggested approach, reinit() now explicitly checks for a read-only buffer, falling back to silently performing a copy if the returned buffer is read-write. That seems vastly preferable to throwing an exception, which would probably be another interface change.

• Call unshare() any place the buffer is about to be modified. If the buffer needs to be made private, it also accepts a size hint indicating how much less/more space the subsequent operation needs, to avoid a redundant reallocation after the unsharing.

Outstanding issues:

• I don't understand why buf_size is a size_t, and I'm certain the casting in unshare() is incorrect somehow. Is it simply to avoid signed overflow?

[dw]

New patch also calls unshare() during getbuffer()

[dw]

I'm not sure the "read only buffer" test is strong enough: having a readonly view is not a guarantee that the data in the view cannot be changed through some other means, i.e. it is read-only, not immutable.

Pretty sure this approach is broken. What about the alternative approach of specializing for Bytes?

[pitrou]

Pretty sure this approach is broken. What about the alternative approach of specializing for Bytes?

That would certainly sound good enough, to optimize the common case.

Also, it would be nice if you could add some tests to the patch (e.g. to stress the bytearray case). Thank you!

[pitrou]

As for whether the "checking for a readonly view" approach is broken, I don't know: that part of the buffer API is still mysterious to me. Stefan, would you have some insight?

[skrah]

I think checking for a readonly view is fine. The protocol is this:

1. Use the PyBUF_WRITABLE flag in the request. Then the provider must
   either have a writable buffer or else deny the request entirely.

2. Omit the PyBUF_WRITABLE flag in the request. Then the provider can
   return a writable or a readonly buffer, but must set the readonly flag
   correctly AND export the same type of buffer to ALL consumers.

It is not possible to ask for a readonly buffer explicitly, but the
readonly flag in the Py_Buffer struct should always be set correctly.

It is hard to guess the original intention of the PEP-3118 authors, but
in practice "readonly" means "immutable" here. IMO a buffer provider would
be seriously broken if a readonly buffer is mutated in any way.

[skrah]

The original wording in the PEP is this:

readonly
--------
an integer variable to hold whether or not the memory is readonly. 1
means the memory is readonly, zero means the memory is writable.

To me this means that a hypothetical compiler that could figure
out at compile time that the readonly flag is set would be allowed
to put the buffer contents into the read-only data section.

[dw]

Stefan,

Thanks for digging here. As much as I'd love to follow this interpretation, it simply doesn't match existing buffer implementations, including within the standard library.

For example, mmap.mmap(..., flags=mmap.MAP_SHARED, prot=mmap.PROT_READ) will produce a read-only buffer, yet mutability is entirely at the whim of the operating system. In this case, "immutability" may be apparent for years, until some machine has memory pressure, causing the shared mapping to be be flushed, and refreshed from (say, incoherent NFS storage) on next access.

I thought it would be worth auditing some of the most popular types of buffer just to check your interpretation, and this was the first, most obvious candidate.

Any thoughts? I'm leaning heavily toward the Bytes specialization approach