Skip to content

gh-128213: fast path for bytes creation from list and tuple #128214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

gh-128213: fast path for bytes creation from list and tuple #128214

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f699b5
gh-128213: fast path for bytes creation from list and tuple
blhsing Dec 24, 2024
18c8e4a
coerce long to char after validation of integer in byte range
blhsing Dec 24, 2024
406fbdb
📜🤖 Added by blurb_it.
blurb-it[bot] Dec 24, 2024
4912a05
Update 2024-12-24-08-44-49.gh-issue-128213.Y71jDi.rst
blhsing Dec 24, 2024
56f802e
updated for thread-safety, style choices, function choices and benchm…
blhsing Dec 25, 2024
4e1e3e6
revert to PyLong_AsLongAndOverflow for easier overflow handling
blhsing Dec 25, 2024
bf96d06
fixed issue of a label at the end of a compound statment; revert to u…
blhsing Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2024-12-24-08-44-49.gh-issue-128213.Y71jDi.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Speed up :class:`bytes` creation from :class:`list` and :class:`tuple` by around 81%.
Patch by Ben Hsing
89 changes: 22 additions & 67 deletions Objects/bytesobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2810,79 +2810,33 @@ _PyBytes_FromBuffer(PyObject *x)
}

static PyObject*
_PyBytes_FromList(PyObject *x)
_PyBytes_FromSequence(PyObject *x)
{
Py_ssize_t i, size = PyList_GET_SIZE(x);
Py_ssize_t value;
char *str;
PyObject *item;
_PyBytesWriter writer;

_PyBytesWriter_Init(&writer);
str = _PyBytesWriter_Alloc(&writer, size);
if (str == NULL)
return NULL;
writer.overallocate = 1;
size = writer.allocated;

for (i = 0; i < PyList_GET_SIZE(x); i++) {
item = PyList_GET_ITEM(x, i);
Py_INCREF(item);
value = PyNumber_AsSsize_t(item, NULL);
Py_DECREF(item);
if (value == -1 && PyErr_Occurred())
goto error;

if (value < 0 || value >= 256) {
PyErr_SetString(PyExc_ValueError,
"bytes must be in range(0, 256)");
goto error;
}

if (i >= size) {
str = _PyBytesWriter_Resize(&writer, str, size+1);
if (str == NULL)
return NULL;
size = writer.allocated;
}
*str++ = (char) value;
}
return _PyBytesWriter_Finish(&writer, str);

error:
_PyBytesWriter_Dealloc(&writer);
return NULL;
}

static PyObject*
_PyBytes_FromTuple(PyObject *x)
{
PyObject *bytes;
Py_ssize_t i, size = PyTuple_GET_SIZE(x);
Py_ssize_t value;
char *str;
PyObject *item;

bytes = PyBytes_FromStringAndSize(NULL, size);
Py_ssize_t size = PySequence_Fast_GET_SIZE(x);
PyObject *bytes = PyBytes_FromStringAndSize(NULL, size);
if (bytes == NULL)
return NULL;
str = ((PyBytesObject *)bytes)->ob_sval;

for (i = 0; i < size; i++) {
item = PyTuple_GET_ITEM(x, i);
value = PyNumber_AsSsize_t(item, NULL);
if (value == -1 && PyErr_Occurred())
char *s = PyBytes_AS_STRING(bytes);
PyObject **items = PySequence_Fast_ITEMS(x);
for (Py_ssize_t i = 0; i < size; i++) {
if (!PyLong_CheckExact(items[i])) {
Py_DECREF(bytes);
return Py_None; // None as fallback sentinel to the slow path
}
int overflow;
long value = PyLong_AsLongAndOverflow(items[i], &overflow);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're still assuming a long object,. we can just use PyLong_AsInt instead. We don't care about an overflow as we're only interested in values in [0, 255].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I copied this code from bytearray but you're absolutely right that PyLong_AsInt is a much better fit here.

Copy link
Contributor Author

@blhsing blhsing Dec 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An existing test with -sys.maxsize failed with PyLong_AsInt. A fix would be to check for OverflowError and reraise ValueError instead, but I think it's easier to simply revert to PyLong_AsLongAndOverflow and let the range check that follows raise ValueError when out of the byte range.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we used PyNumber_AsSsize_t (which invokes _PyNumber_Index, hence possibly creating side effects). This will be a small behavioral change. While I understand Guido's comment, I'm wondering whether we should keep the old behaviour (though I don't know how it could be useful in production, namely making bytes() have side-effects on lists if one of their element being converted invokes __index__).

Note that the current code also prevented crashes by temporarily increfing item before calling PyNumber_AsSsize_t.

Copy link
Contributor Author

@blhsing blhsing Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR is not supposed to be about altering behaviors, I'll keep the current behavior for this PR and file a separate bug against the current behavior then. It should be considered a bug because like Guido pointed out, a list can potentially be mutated inside __index__, resulting in the item copy loop accessing freed memory. Despite the unlikelihood that such code exists in the real world, it can still potentially happen and cause a crash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have a crash because the list size is checked at every iteration (though what should be checked perhaps is that the list pointer is not NULL). We incref the item before calling __index__ on it so it shouldn't cause crashes. INCREFing before is a trick you also use for use-after-free issues and evil mutations (but we can easily check if this is crashing or not as follows:

class EvilInt:
	def __index__(self):
		x.clear()
		return 0

x = [1,2, EvilInt(), 4]
bytes(x)

and this does not crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh now I see what INCREF and DECREF are doing there. I'll add them back in a bit too then. Thanks.

if (value == -1 && PyErr_Occurred()) {
goto error;

}
if (value < 0 || value >= 256) {
/* this includes an overflow in converting to C long */
PyErr_SetString(PyExc_ValueError,
"bytes must be in range(0, 256)");
goto error;
}
*str++ = (char) value;
s[i] = (char)value;
}
return bytes;

error:
Py_DECREF(bytes);
return NULL;
Expand Down Expand Up @@ -2968,11 +2922,12 @@ PyBytes_FromObject(PyObject *x)
if (PyObject_CheckBuffer(x))
return _PyBytes_FromBuffer(x);

if (PyList_CheckExact(x))
return _PyBytes_FromList(x);

if (PyTuple_CheckExact(x))
return _PyBytes_FromTuple(x);
if (PyList_CheckExact(x) || PyTuple_CheckExact(x)) {
PyObject *bytes = _PyBytes_FromSequence(x);
if (bytes != Py_None) {
return bytes;
}
}

if (!PyUnicode_Check(x)) {
it = PyObject_GetIter(x);
Expand Down
Loading