PyBytes_Concat could try to concat in-place

[pitrou]

BPO	21377
Nosy	@pitrou, @vstinner
Files	issue_21377.diff issue_21377_r2.diff issue_21377_r3.diff issue_21377_r4.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-05-01.12:43:11.785>
created_at = <Date 2014-04-28.19:47:24.270>
labels = ['interpreter-core', 'performance']
title = 'PyBytes_Concat could try to concat in-place'
updated_at = <Date 2014-05-06.16:43:11.936>
user = 'https://github.com/pitrou'

bugs.python.org fields:

activity = <Date 2014-05-06.16:43:11.936>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2014-05-01.12:43:11.785>
closer = 'pitrou'
components = ['Interpreter Core']
creation = <Date 2014-04-28.19:47:24.270>
creator = 'pitrou'
dependencies = []
files = ['35084', '35085', '35100', '35123']
hgrepos = []
issue_num = 21377
keywords = ['patch']
message_count = 8.0
messages = ['217406', '217462', '217480', '217498', '217499', '217694', '217695', '217994']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'nikratio', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue21377'
versions = ['Python 3.5']

[pitrou]

Currently, PyBytes_Concat always creates a new bytes object for the result. However, when Py_REFCNT(*pv) == 1, it could instead call _PyBytes_Resize() and then concat the second argument in place.

(like e.g. _PyUnicode_Append does)

[nikratio]

Tentative patch attached. The test suite still passes, but I'm not sure if it actually exerts the new code path. Is there a standard way to test the C api?

[pitrou]

Tentative patch attached. The test suite still passes, but I'm not
sure if it actually exerts the new code path.

A quick grep shows me that it should be exercised at least by Modules/_io/bufferedio.c.
Otherwise, the way to test the C API is to add a function to Modules/_testcapi.c, and test that function from the relevant test file.

[vstinner]

If I remember correctly, ceval.c has an optmisation for str += str even if
the refcount is 2. Do we need to implement it or suggest to use bytearray
or b''.join() instead?

[pitrou]

If I remember correctly, ceval.c has an optmisation for str += str even if
the refcount is 2. Do we need to implement it or suggest to use bytearray
or b''.join() instead?

The latter, IMO. This issue is about the C API _PyBytes_Concat.

[pitrou]

Thanks! The latest patch looks fine to me.

[python-dev]

New changeset 4ed1b6c7e2f3 by Antoine Pitrou in branch 'default':
Issue bpo-21377: PyBytes_Concat() now tries to concatenate in-place when the first argument has a reference count of 1.
http://hg.python.org/cpython/rev/4ed1b6c7e2f3

[python-dev]

New changeset 6234f4caba57 by Zachary Ware in branch 'default':
Issue bpo-21442: Fix MSVC compiler warning introduced by bpo-21377.
http://hg.python.org/cpython/rev/6234f4caba57