Remove copy from bytearray to bytes in rawiobase_read() implementation

@jcea

BPO	15903
Nosy	@jcea, @amauryfa, @pitrou, @vstinner, @methane
Files	iobase_read.patch iobase_read.patch

^{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 = None
created_at = <Date 2012-09-10.12:23:03.370>
labels = ['library', 'expert-IO', 'performance']
title = 'Make rawiobase_read() read directly to bytes object'
updated_at = <Date 2019-04-08.13:06:30.879>
user = 'https://bugs.python.org/sbt'

bugs.python.org fields:

activity = <Date 2019-04-08.13:06:30.879>
actor = 'BreamoreBoy'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)', 'IO']
creation = <Date 2012-09-10.12:23:03.370>
creator = 'sbt'
dependencies = []
files = ['27166', '27211']
hgrepos = []
issue_num = 15903
keywords = ['patch']
message_count = 20.0
messages = ['170183', '170226', '170624', '170627', '170628', '170629', '170638', '170639', '170642', '170658', '170659', '170660', '170675', '170676', '170678', '170679', '170680', '170681', '190457', '223225']
nosy_count = 7.0
nosy_names = ['jcea', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'methane', 'sbt', 'dwight.guth']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue15903'
versions = ['Python 3.4', 'Python 3.5']

Linked PRs

Currently rawiobase_read() reads to a bytearray object and then copies the data to a bytes object.

There is a TODO comment saying that the bytes object should be created directly. The attached patch does that.

It works as along as the bytes object cannot leak to Python code, (imagine a custom readinto() method which plays with gc.get_referrers, then calls hash(b)...)
This is OK with this patch.

New patch which checks the refcount of the memoryview and bytes object after calling readinto().

If either refcount is larger than the expected value of 1, then the data is copied rather than resized.

If either refcount is larger than the expected value of 1, then the
data is copied rather than resized.

I think that's a useless precaution. The bytes object cannot "leak" since you are using PyMemoryView_FromMemory(), which doesn't know about the original object.

Out of curiousity, have you done any benchmarks?

I think that's a useless precaution. The bytes object cannot "leak"
since you are using PyMemoryView_FromMemory(), which doesn't know about
the original object.

The bytes object cannot "leak" so, as you say, checking that refcount is pointless. But the view might "leak", and since it does not own a reference to the base object we have a problem: we can't deallocate the bytes object for fear of breaking the view.

It looks like objects returned by PyMemoryView_FromMemory() must never be allowed to "leak", so I am not sure there are many circumstances in which PyMemoryView_FromMemory() is safe to use.

Perhaps using PyBuffer_FillInfo() and PyMemory_FromBuffer() would keep alive the bytes object while the view is alive, without letting the bytes object "leak".

Out of curiousity, have you done any benchmarks?

No.

The bytes object cannot "leak" so, as you say, checking that refcount
is pointless. But the view might "leak", and since it does not own a
reference to the base object we have a problem: we can't deallocate the
bytes object for fear of breaking the view.

Indeed, that's a problem (but your patch does deallocate the bytes object).
It's quite fishy, I'm not sure how to solve the issue cleanly. Stefan, do you have an idea?

So the problem is that readinto(view) might result in several references
to view? I don't think that can be solved on the memoryview side.

One could do:

   view = PyMemoryView_FromObject(b);
   // Lie about writability
   ((PyMemoryViewObject *)view)->view.readonly = 0;

[...]

Then the view owns a reference to the bytes object. But that does not
solve the problem that writable memoryviews based on a readonly object
might be hanging around.

Then the view owns a reference to the bytes object. But that does not
solve the problem that writable memoryviews based on a readonly object
might be hanging around.

How about doing

    PyObject_GetBuffer(b, &buf, PyBUF_WRITABLE);
    view = PyMemoryView_FromBuffer(&buf);
    // readinto view
    PyBuffer_Release(&buf);

Would attempts to access a "leaked" reference to view now result in ValueError("operation forbidden on released memoryview object")? If so then I think this would be safe.

The current non-test uses of PyMemoryView_FromBuffer() are in _io.BufferedReader.read(), _io.BufferedWriter.write(), PyUnicode_Decode().

It looks like they can each be made to leak a memoryview that references a deallocated buffer. (Maybe the answer is Don't Do That.)

import codecs, sys

def decode(buf):
    global view
    view = buf
    return codecs.latin_1_decode(buf)

def getregentry():
    return codecs.CodecInfo(name='foobar', decode=decode,
                            encode=codecs.latin_1_encode)

@codecs.register
def search_function(encoding):
    if encoding == 'foobar':
        return codecs.CodecInfo(*getregentry())

b = b'hello'.upper()
b.decode('foobar')
print(view.tobytes())                   # => b'HELLO'
del b
x = b'dump'.upper()
print(view.tobytes())                   # => b'DUMP\x00'



import io, sys

class File(io.RawIOBase):
    def readinto(self, buf):
        global view
        view = buf
        n = len(buf)
        buf[:] = b'x'*n
        return n

    def readable(self):
        return True

f = io.BufferedReader(File())
f.read(1)
print(view[:5].tobytes())       # => b'xxxxx'
del f
print(view[:5].tobytes())       # => b'\xdd\xdd\xdd\xdd\xdd'

I am rather confused about the ownership semantics when one uses PyMemoryView_FromBuffer().

It looks as though PyMemoryView_FromBuffer() "steals" ownership of the buffer since, when the associated _PyManagedBufferObject is garbage collected, PyBuffer_Release() is called on its copy of the buffer info. However, the _PyManagedBufferObject does not own a reference of the base object, so one still needs to decref the base object (at some time when it is safe to do so).

So am I right in thinking that

  PyObject_GetBuffer(obj, &buf, ...);
  view = PyMemoryView_FromBuffer(&buf);     // view->master owns the buffer, but view->master->obj == NULL
  ...
  Py_DECREF(view);                          // releases buffer (assuming no other exports)
  Py_XDECREF(buf.obj);

has balanced refcounting and is more or less equivalent to

  view = PyMemoryView_FromObject(obj);
  ...
  Py_DECREF(view);

The documentation is not very helpful. It just says that calls to PyObject_GetBuffer() must be matched with calls to PyBuffer_Release().

Richard Oudkerk <report@bugs.python.org> wrote:

PyObject_GetBuffer(b, &buf, PyBUF_WRITABLE);
view = PyMemoryView_FromBuffer(&buf);
// readinto view
PyBuffer_Release(&buf);
Would attempts to access a "leaked" reference to view now result in ValueError("operation forbidden on released memoryview object")? If so then I think this would be safe.

You would need to call memory_release(). Perhaps we can just expose it on the
C-API level as PyMemoryView_Release().

IMO the use of PyObject_GetBuffer() should be discouraged. The semantics
aren't clear (see bpo-15821). I'd suggest using:

A buffer provider is involved (the case here):

PyMemoryView_From Object()
A piece of memory needs to be wrapped temporarily and no references
to the memoryview are "leaked" on the Python level:

        PyMemoryView_FromMemory()

A piece of memory needs to be packaged as a memoryview with automatic
cleanup in mbuf_dealloc():

PyMemoryView_FromBufferWithCleanup() (proposed in msg169613)

So I think the combination of PyMemoryView_FromObject() with a call to
PyMemoryView_Release() should indeed work here.

Richard Oudkerk <report@bugs.python.org> wrote:

The documentation is not very helpful. It just says that calls
to PyObject_GetBuffer() must be matched with calls to PyBuffer_Release().

Yes, we need to sort that out, see bpo-15821.

Please note this is also referred to from bpo-15994.

Another approach here is to make it so bytearray can be converted to bytes without a copy. Looking to see if can use _io.BytesIO in this case / performance is reasonable with that which already exposes the "mutable buffer" -> bytes without copying.

pythongh-60107: pythongh-138616: Minor code changes

pythongh-60107: Change blurb_it

pythongh-60107: Tests for readinto

pythongh-60107: Remove a copy from RawIOBase.read

gh-60107: Remove a copy from RawIOBase.read (#141532)

pythongh-60107: Remove a copy from RawIOBase.read (python#141532)

Navigation Menu

Repository navigation

Remove copy from `bytearray` to `bytes` in rawiobase_read() implementation #60107

Linked PRs

7 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Remove copy from bytearray to bytes in rawiobase_read() implementation #60107

Description

Linked PRs

Activity

sbt commented on Sep 10, 2012

amauryfa commented on Sep 10, 2012

sbt commented on Sep 17, 2012

pitrou commented on Sep 17, 2012

sbt commented on Sep 17, 2012

pitrou commented on Sep 17, 2012

skrah commented on Sep 18, 2012

sbt commented on Sep 18, 2012

sbt commented on Sep 18, 2012

sbt commented on Sep 18, 2012

skrah commented on Sep 18, 2012

skrah commented on Sep 18, 2012

7 remaining items

BreamoreBoy commented on Jul 16, 2014

cmaloney commented on May 19, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Remove copy from `bytearray` to `bytes` in rawiobase_read() implementation #60107